Re: [PATCH 2/4] ntdll: If read_directory_getattrlist() finds that the file doesn't exist, return successful "no file" result to stop search.

[Matteo Bruni]

----
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
index 0e5af8a..42e1842 100644
--- a/dlls/ntdll/directory.c
+++ b/dlls/ntdll/directory.c
@@ -2185,6 +2185,11 @@ static int read_directory_getattrlist( int fd,
IO_STATUS_BLOCK *io, void *buffer
             }
             else io->u.Status = STATUS_NO_MORE_FILES;
         }
+        else if (errno == ENOENT || errno == ENOTDIR)
+        {
+            io->u.Status = STATUS_NO_MORE_FILES;
+            ret = 0;
+        }
     }
     else ret = -1;
----

Actually that's how my patch originally looked like
(http://source.winehq.org/patches/data/111375) but then I realized
that would break if the filesystem is case-sensitive, since it would
return the file as missing while it might be present with a different
name casing. I haven't actually tested it though.

[PATCH 2/4] ntdll: If read_directory_getattrlist() finds that the file doesn't exist, return successful "no file" result to stop search.

[Ken Thomases]

On May 20, 2015, at 2:38 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> ----
> diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
> index 0e5af8a..42e1842 100644
> --- a/dlls/ntdll/directory.c
> +++ b/dlls/ntdll/directory.c
> @@ -2185,6 +2185,11 @@ static int read_directory_getattrlist( int fd,
> IO_STATUS_BLOCK *io, void *buffer
>             }
>             else io->u.Status = STATUS_NO_MORE_FILES;
>         }
> +        else if (errno == ENOENT || errno == ENOTDIR)
> +        {
> +            io->u.Status = STATUS_NO_MORE_FILES;
> +            ret = 0;
> +        }
>     }
>     else ret = -1;
> ----
>
> Actually that's how my patch originally looked like
> (http://source.winehq.org/patches/data/111375) but then I realized
> that would break if the filesystem is case-sensitive, since it would
> return the file as missing while it might be present with a different
> name casing. I haven't actually tested it though.

Good point.  I can add a check for the case-sensitivity of the file system.

-Ken

[PATCH 2/4] ntdll: If read_directory_getattrlist() finds that the file doesn't exist, return successful "no file" result to stop search.

[Matteo Bruni]

2015-05-20 21:44 GMT+02:00 Ken Thomases <ken(a)codeweavers.com>:
> On May 20, 2015, at 2:38 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> > ----
> > diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
> > index 0e5af8a..42e1842 100644
> > --- a/dlls/ntdll/directory.c
> > +++ b/dlls/ntdll/directory.c
> > @@ -2185,6 +2185,11 @@ static int read_directory_getattrlist( int fd,
> > IO_STATUS_BLOCK *io, void *buffer
> >             }
> >             else io->u.Status = STATUS_NO_MORE_FILES;
> >         }
> > +        else if (errno == ENOENT || errno == ENOTDIR)
> > +        {
> > +            io->u.Status = STATUS_NO_MORE_FILES;
> > +            ret = 0;
> > +        }
> >     }
> >     else ret = -1;
> > ----
> >
> > Actually that's how my patch originally looked like
> > (http://source.winehq.org/patches/data/111375) but then I realized
> > that would break if the filesystem is case-sensitive, since it would
> > return the file as missing while it might be present with a different
> > name casing. I haven't actually tested it though.
>
> Good point.  I can add a check for the case-sensitivity of the file system.
>
> -Ken

Yeah, that's a possibility. My thought was that it might be better to
simply fallback to the other methods in that case instead of adding
another syscall (you can see that as an optimization for the default
case-insensitive filesystems at the price of worse performance on
case-sensitive FSs). I'm just speculating though. Either is fine to
me, your call.

BTW thank you for fixing those bugs!

[PATCH 2/4] ntdll: If read_directory_getattrlist() finds that the file doesn't exist, return successful "no file" result to stop search.

[Ken Thomases]

On May 20, 2015, at 3:09 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> 2015-05-20 21:44 GMT+02:00 Ken Thomases <ken(a)codeweavers.com>:
> > Good point.  I can add a check for the case-sensitivity of the file system.
>
> Yeah, that's a possibility. My thought was that it might be better to
> simply fallback to the other methods in that case instead of adding
> another syscall (you can see that as an optimization for the default
> case-insensitive filesystems at the price of worse performance on
> case-sensitive FSs). I'm just speculating though. Either is fine to
> me, your call.

Falling back to the other methods seems to be invariably worse than just checking in read_directory_getattrlist().  For example, read_directory_stat() has the same problem, so it (now) requires a call to get_dir_case_sensitivity(), anyway, plus the call to stat().

I suppose there may be some room for optimization by extracting the part of get_dir_case_sensitivity_attr() after the lookup of ATTR_CMN_DEVID and ATTR_CMN_FSID to a separate function.  Then, we can look up those attributes as part of the primary getattrlist() call in read_directory_getattrlist() and avoid having to do another call to get them for checking case sensitivity.
> BTW thank you for fixing those bugs!

You're welcome.  Thanks for doing the initial work that I was too lazy to do. ;)

-Ken