delorie.com/archives/browse.cgi   search  
Mail Archives: djgpp-workers/2001/10/25/06:40:56

Message-ID: <013901c15d41$34fae5c0$b07224d5@zephyr>
From: "Eric Botcazou" <ebotcazou AT libertysurf DOT fr>
To: "DJGPP workers" <djgpp-workers AT delorie DOT com>
References: <Pine DOT SUN DOT 3 DOT 91 DOT 1011024104311 DOT 21151C-100000 AT is>
Subject: Re: _findfirst() patch (2)
Date: Thu, 25 Oct 2001 12:37:52 +0200
MIME-Version: 1.0
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

- Raw text -


  webmaster     delorie software   privacy  
  Copyright © 2019   by DJ Delorie     Updated Jul 2019