Date: Wed, 24 Oct 2001 10:43:40 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: Eric Botcazou cc: DJGPP workers Subject: Re: _findfirst() patch (2) In-Reply-To: <001c01c15bdf$cac187c0$0d2c24d5@zephyr> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk 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 > #include > #include > -#include 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 > #include > > -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?