Message-ID: <013901c15d41$34fae5c0$b07224d5@zephyr> From: "Eric Botcazou" To: "DJGPP workers" References: Subject: Re: _findfirst() patch (2) Date: Thu, 25 Oct 2001 12:37:52 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 5.00.2014.211 X-MimeOLE: Produced By Microsoft MimeOLE V5.00.2014.211 Reply-To: djgpp-workers AT delorie DOT com > Why did you remove string.h? The code uses strlen, so string.h must > be included. Stupid mistake ! gcc 2.95.3 doesn't complain though but I guess gcc 3.0.x will. > > +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. It seems I was overzealous here, but most of the source files I looked at use this 'two lines' layout. However, others don't so I guess this is a somewhat wavering convention. > > __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. That's not very clear... See for instance dos/dir/findfirs.c (EACCES), dos/dir/findnext.c (EACCES), dos/dir/freewlk.c (EFAULT), dos/dir/ftw.c (EFAULT), etc > 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. I don't know, I tried to make the two low-level functions (_dos_findfirst and _lfn_findfirst) consistent: success in the same circumstances, failure in the same circumstances. > > + 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). No, of course, but that's consistent with the errno code since _doserr_to_errno(5) = EACCES. > > - r.x.dx = (__tb & 15) + strlen(name) + 1; > > - r.x.ds = __tb / 16; > > - r.h.ah = 0x1A; > > > > + 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. Well, this may be a gratuitous change for expert DOS programmers like you, but IMHO very helpful for amateurs like me who don't always have in mind the equivalence formula between linear addresses and segment/offset pairs. Moreover, why to bother defining symbols in order not to use them ? > > - 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. Sorry, I've done too much Allegro coding recently :-) > > - 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. I could turn the question... See dos/dir/findfirs.c and dos/dir/findnext.c . Consistency again. > > -void _dos_gettime(struct _dostime_t *time) > > +void _dos_gettime(struct _dostime_t *dtime) > > Why did you need to make this change? Because I wanted to make a bigger patch ;-) More seriously, 'time' conflicts with the time() function from time.h Sorry, I forgot to mention it in my message. > > + 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. dos/dir/findfirs.c and dos/dir/findnext.c use 0 for closed handles, so that a zero handle means 'terminated search'. It seems that it has worked up to now. > > + 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. See the previous comment. > > +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)? Why ? You must not pass an invalid handle to the function. > 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. _lfn_findfirst() is not supposed to be directly called in my mind so the problem should never occur. But you're right, I can add an additional check. > `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'. Copy-pasted from dos/compat/d_findf.txh > 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. I would say it's overkill to protect the _lfn functions against invalid handles: the new findfirst() already checks the handle, as for _findfirst() the user *must* check the returned handle. I'll test Windows's behaviour anyway. > 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? I can document the -1 return value. > 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. Ok, what's your method ? > Are you sure you want the creation and access time members to be set > to -1? Is that what Watcom does? Yes, the docs say: -1 for FAT file systems. > > +@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). Copy-pasted from dos/compat/d_findf.txh > 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? Why ? the user must call _findclose() in all cases. findnext() doesn't mention ENMFILE either and it's exactly the same situation. > > +#define _sizeof_dos_ffblk 44 > > Don't we have some struct whose sizeof we should take here? Copy-pasted from dos/dir/findfirs.c and dos/dir/findnext.c Using it instead of a mere sizeof() saves a lot of bytes. -- Eric Botcazou ebotcazou AT multimania DOT com