Mail Archives: djgpp-workers/2001/10/24/04:45:50
On Tue, 23 Oct 2001, Eric Botcazou wrote:
> I've attached a (completely) revised version of my _findfirst() patch,
> following the guidelines Eli and Charles provided me with.
Thanks!
I have a few comments.
> +++ /djgpp-cvs/src/libc/dos/compat/d_findf.c Tue Oct 23 17:15:08 2001
> @@ -15,29 +15,46 @@
> #include <go32.h>
> #include <errno.h>
> #include <dpmi.h>
> -#include <string.h>
Why did you remove string.h? The code uses strlen, so string.h must
be included.
> +unsigned int
> +_dos_findfirst(const char *name, unsigned int attr, struct _find_t *result)
Please in the future try to avoid gratuitous formatting changes such
as this one: they clutter the diffs and make them harder to review,
because the real changes are less visible.
> {
> __dpmi_regs r;
> + int namelen;
> +
> + if (name == 0 || result == 0) {
> + errno = EACCES;
I believe our policy is to set errno to EINVAL when supplied argument
pointers are invalid.
More importantly, does the MSC version fail such cases?
_dos_findfirst is a compatibility function, so I think we shouldn't do
anything the originals don't.
> + return 0x05; /* DOS error code: access denied */
Does _dos_findfirst indeed return 5 in this case? I'd expect 2 or 3
(File Not Found or Path Not Found).
> _put_path(name);
> - r.x.dx = (__tb & 15) + strlen(name) + 1;
> - r.x.ds = __tb / 16;
> - r.h.ah = 0x1A;
> +
> + /* There will be a _sizeof_dos_ffblk character return value from _dos_findfirst
> + in the DTA. Put the file name before this. First set the DTA to be
> + transfer buffer. */
> +
> + r.x.dx = __tb_offset + namelen;
> + r.x.ds = __tb_segment;
> + r.h.ah = 0x1a;
Except for the comment, this is also a gratuitous change. Please try
to avoid those.
> - if ( r.x.flags & 1 )
> - {
> +
> + if (r.x.flags&1) { /* error? */
> errno = __doserr_to_errno(r.x.ax);
> return r.x.ax;
> }
Ditto: the original code used the indentation style that is standard
in the DJGPP library; please keep that style intact.
> - dosmemget(__tb + strlen(name) + 1, sizeof(struct _find_t), result);
> +
> + /* Recover results */
> + dosmemget(__tb + namelen, _sizeof_dos_ffblk, result);
This change could actually be confusing: since you are filling the
_find_t struct, why not use its size? sizeof is a compile-time
operator, so no run-time penalties are involved.
> +++ /djgpp-cvs/src/libc/dos/compat/d_gettim.c Mon Oct 22 20:40:02 2001
> @@ -12,14 +12,14 @@
> #include <dpmi.h>
> #include <dos.h>
>
> -void _dos_gettime(struct _dostime_t *time)
> +void _dos_gettime(struct _dostime_t *dtime)
Why did you need to make this change?
> + handle = _lfn_findfirst(pathname, attrib, &ffblk32);
> + if (handle > 0) {
Are we sure that the handle can never be zero? My references don't
seem to say that? Perhaps handle >= 0 is better.
> + if (!strchr(pathname,'*') && !strchr(pathname,'?')) {
> + _lfn_findclose(handle);
> + handle = 0;
> }
This (and quite a few of other places) use style of braces that is
different from what is customary throughout the library.
> + if (!ffblk->lfn_handle) { /* handle already closed? */
[...]
> + ffblk->lfn_handle = 0;
Unless we are sure the handle can never be zero, better use -1 for a
closed handle, I think.
> +int
> +_lfn_findclose(long handle)
> +{
> + __dpmi_regs r;
> +
> + r.x.ax = 0x71a1;
> + r.x.bx = handle;
> + __dpmi_int(0x21, &r);
Shouldn't we see if handle is invalid (negative)?
> + r.x.ax = 0x714e;
> + r.x.cx = attrib;
> + r.x.dx = __tb_offset;
> + r.x.ds = __tb_segment;
> + r.x.di = __tb_offset + pathlen;
> + r.x.es = r.x.ds;
> + r.x.si = USEDOSDATE;
> + __dpmi_int(0x21, &r);
> +
> + if (r.x.flags&1) { /* error? */
> + errno = __doserr_to_errno(r.x.ax);
> + return -1;
> + }
I'm not sure this is correct: does __doserr_to_errno produce ENOSYS if
714Eh returns with 7100h in AX, as it does on plain DOS? I think it
doesn't, in which case we need to generate ENOSYS manually.
> +This function and the related @code{_lfn_findnext} =
> (@pxref{_lfn_findnext})
> +and @code{_lfn_findclose} (@pxref{_lfn_findclose}) are used to scan
> +directories for the list of files therein. However they are not =
Please leave two blanks after the period which ends a sentence. This
is important if someone wants to generate a printed version of the
library docs: TeX, the program used to typeset the manual, knows about
the 2-blanks-end-sentence rule, and produces more pleasant results in
print.
> +to be directly called. Use one of the two public functions instead
> +(@pxref{findfirst} and @pxref{_findfirst}).
This is a wrong usage of @pxref, it produces awkward or even
unreadable results. Instead, say this:
Use one of the two public functions: @code{findfirst} (@pxref{findfirst})
and @code{findnext} (@pxref{_findfirst}).
> +@code{_lfn_findfirst} returns -1 (and sets @var{errno}).
`errno' should have the @code markup, not @var. @var should be only
used for formal parameters or for other entities which stand for
something, whereas `errno' is a proper identifier, like `findfirst'.
> +int
> +_lfn_findnext(long handle, struct ffblklfn *ffblklfn)
> +{
> + __dpmi_regs r;
> +
> + if (ffblklfn == 0) {
> + errno = EACCES;
> + return -1;
> + }
> +
> + r.x.ax = 0x714f;
> + r.x.bx = handle;
I think we need protection against invalid handles here. Could you
please see how Windows reacts to invalid (negative) handles? This
could be Windows version dependent, so perhaps you could throw
together a short test program, and post it here, so that people could
run it on different versions of Windows and report the results.
> + memset(&t, 0, sizeof(struct tm));
> + t.tm_sec = ftimep->ft_tsec * 2;
> + t.tm_min = ftimep->ft_min;
> + t.tm_hour = ftimep->ft_hour;
> + t.tm_mday = ftimep->ft_day;
> + t.tm_mon = ftimep->ft_month - 1;
> + t.tm_year = ftimep->ft_year + 80;
> + t.tm_isdst = -1;
> +
> + return mktime(&t);
mktime can return -1 if the values in its argument are invalid. Are
the users of _ftime_to_time_t capable of coping with that?
> +int
> +_findclose(long handle)
> +{
> + if (_USE_LFN) {
> + return _lfn_findclose(handle);
> + }
> + else {
> + free((struct ffblk*)handle);
> + return 0;
I think you want to test handle for being a valid pointer (if you
don't know how, I can suggest a way). Otherwise, a call to _findclose
with a bogus (negative) value could corrupt the malloc chain.
> + if (_dos_findfirst(pathname, ALL_ATTRIB, (struct _find_t *)ffblk16) == 0) {
> + fileinfo->attrib = (unsigned)ffblk16->ff_attrib;
> + fileinfo->time_create = -1;
> + fileinfo->time_access = -1;
> + fileinfo->time_write = _ftime_to_time_t((struct ftime *)&ffblk16->ff_ftime);
Are you sure you want the creation and access time members to be set
to -1? Is that what Watcom does? I'd suggest setting them to the
same value as the last write time.
> +@item _A_ARCH (0x20)
> +
> +Archive file
That's not accurate: the archive bit means that the file was modified
since last backup (or other operation, such as "attrib -a", which
resets that bit).
> +This function supports long file names.
It supports long file names when they are available in the underlying
OS.
> +int _findnext(long handle, struct _finddata_t *fileinfo);
> +@end example
> +
> +@subheading Description
> +
> +This finds the next file in the search started by @code{_findfirst}.
> +See @ref{_findfirst}, for the description of @code{struct _finddata_t}.
> +
> +This function supports long file names.
> +
> +@subheading Return Value
> +
> +Zero if a match is found, -1 if not found (and sets @var{errno}).
Don't you want to mention ENMFILE here? Since _findnext doesn't close
the handle automatically, users will need to know about that special
value of errno, right?
> +#define _sizeof_dos_ffblk 44
Don't we have some struct whose sizeof we should take here?
- Raw text -