Mail Archives: djgpp-workers/2001/10/25/10:02:19
On Thu, 25 Oct 2001, Eric Botcazou wrote:
> > I believe our policy is to set errno to EINVAL when supplied argument
> > pointers are invalid.
Sorry, I meant EFAULT.
> 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
I just loath to see EACCES everywhere; DOS and Windows tend to return it
for anything they don't understand, so I don't think we should add to that.
I'd say lets change all the functions in this family to return EFAULT.
> > > + 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.
I don't think this consistency matters too much. It's more important to
tell the calling application something intelligent about why did the
function fail.
> > > - 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.
The reason for this request is, as I explained, that it makes it easier
to review the changes. There are no other principles beyond that. I
agree that sometimes the old code could be made more readable, but I'm
asking not to mix real changes with cosmetic ones.
> Moreover, why to bother defining symbols in order not to use them ?
IIRC, the original code was written before those macros were added.
> > > - 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.
It's arguable, I think: _dos_findfirst _did_ use sizeof.
> > > +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.
Even if an invalid handle causes the user's disk to be wiped out? ;-)
The point is that these functions manipulate disk files. I wanted them
to exercise caution so that programmatic mistakes won't cause dramatic
consequences.
> > `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
Yes, there are places in the docs where incorrect markup is used. That
doesn't mean we should add new ones.
> > 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.
Please do.
> > 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 ?
The value should be more than __djgpp_stack_limit-_stklen and less than
__djgpp_selector_limit.
> > 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.
Then please document that (or did I miss that?)
> > > +@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
So that file needs to be corrected as well ;-)
> > 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.
No, findnext doesn't require the application to close the handle, it does
that itself. So in the case of findnext, if the application is not
written to handle ENMFILE specially, it will report an error, but with
_findnext, the application will leak search handles (or memory).
> > > +#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.
I don't understand: why does it save a lot of bytes? sizeof isn't a
function.
- Raw text -