Mail Archives: djgpp-workers/2001/10/25/06:40:56
> 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
- Raw text -