Front page | perl.perl5.porters |
Postings from September 2010
----- Original Message -----
From: "Father Chrysostomos via RT" <perlbug-followup@perl.org>
To: <alex.davies@talktalk.net>
Sent: Sunday, September 26, 2010 10:14 PM
Subject: [perl #71714] Fw: PATCH remove redundant stat from Win32's opendir()
> On Tue Dec 29 07:20:20 2009, alex.davies@talktalk.net wrote:
>> > Here's a patch that removes an unnecesary call to win32_stat() by
>> > the opendir code on Win32. This provides a noticeable speed up when
>> > recursively traversing a directory eg. calls to File::Find::find().
>> >
>> > Note it does change behaviour in the following cases:
>> >
>> > 1. The patch makes directory names longer than MAX_PATH fail and
>> > sets errno to ENAMETOOLONG ("Filename too long"). Currently, in
>> > this case errno is not actually set in win32_opendir. Consider:
>> >
>> > c:\> perl -e "$!=0 ; opendir C, ('Q'x300) or die $!
>> > Bad file descriptor at -e line 1.
>> >
>> > c:\> perl -e "$!=11 ; opendir C, ('Q'x300) or die $!
>> > Resource temporarily unavailable at -e line 1.
>
> That’s certainly a bug. I’m all for fixing that.
>
>> > Note that when $!=0, pp_open_dir() has to resort to setting errno
>> > to EBADF. This looks like an oversight in the original code as I
>> > doubt this was the intended error code in this case.
>> >
>> > 2. The patch makes a zero length directory name fail and sets errno
>> > to EINVAL. Currently "" is passed to win32_stat which happens to set
>> > errno to ENOENT (i guess that's consistent over all Windows
>> versions).
>> > This looks like an oversight - why stat a filename that cannot
>> possibly
>> > exist? ie. setting errno to ENOENT here seems accidental. EINVAL
>> > makes more sense. (I can't find a "Filename too short" error code ;-
>> )
>
> In this case, you will be making Windows different from Unix systems in
> yet another case. On Mac OS X I get:
>
> $ perl -le' opendir C, "" or die $!'
> No such file or directory at -e line 1.
>
> I think Windows should do the same.
>
>> > In the above 2 cases, it's straightforward to change the patch code
>> > to maintain backward compatibility. I don't have any great
>> > attachment to changing the error codes and if someone wants to
>> > argue for backwards compatibility i'll not haggle.
>> >
>> > Probably of more interest is...
>> >
>> > 3. If the directory is actually a regular file then currently errno
>> > is not set, and so (as in case 1) errno ends up being set to EBADF.
>> > The patched version will 'fail' via the same code path as if it's
>> > just a non existant file/directory name (FindFirstFile will return
>> > ERROR_PATH_NOT_FOUND) and so errno gets set to ENOENT.
>> >
>> > In this case both error codes are plainly wrong: there's
>> > no file descriptor to be bad, and since it *is* a file it cannot
>> > be said to be "No such file or directory".
>> > Much better would be ENOTDIR ("Not a directory").
>
> Agreed.
>
>> > [Changing the patched version to make a distinction between where
>> the
>> > directory to be opened is neither-a-regular-file-nor-directory
>> (ENOENT)
>> > and is is-not-a-directory-(because-it's-a-regular-file) (ENOTDIR)
>> would
>> > require adding a stat in the ERROR_PATH_NOT_FOUND case.
>> > Or alternatively just use ENOTDIR for both cases - but this would
>> > expose a change in error code in the common case of
>> > neither-a-regular-file-nor-directory.]
>
> It would bring it into line with Unix, which I think would be good, but
> not necessary for getting this patch through.
>
> Could you resubmit the patch without the ‘Filename too short’ change?
>
The requested patch is attached.
Many thanks, alex.
Thread Next