Re: [PATCH v5] ntdll/directory: Add support for EXT4 case folding per directory

[Gabriel Ivăncescu]

On 6/13/19 4:45 PM, Huw Davies wrote:
> ntdll/directory: -> ntdll:
>
> On Thu, Jun 13, 2019 at 02:44:58PM +0300, Gabriel Ivăncescu wrote:
> > This adds support for checking the case-insensitive attribute on ext4
> > with newer kernels so that Wine can rely on it for performance. Checking
> > for the EXT4_CASEFOLD_FL attribute involves an ioctl, which operates on
> > file descriptors, while all the former checks operated directly on the
> > dir pathname (e.g. statfs). Obviously, it's best to avoid looking up the
> > directory multiple times (also for correctness, so it refers to the same
> > dir). So in the case that we *do* have a file descriptor, then use it
> > everywhere, with e.g. fstatfs.
>
> This could be re-worded a bit.  In particular mentioning the file descriptor
> change is unnecessary.
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099
> > Signed-off-by: Gabriel Ivăncescu
> >
> > v5: Only use the file descriptor on Linux. The second patch is not resent
> > since it's in staging for users to test out first before going upstream.
> >
> > dlls/ntdll/directory.c | 44 +++++++++++++++++++++++++++---------------
> >   1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
> > index bbdbbe9..9b4e61b 100644
> > --- a/dlls/ntdll/directory.c
> > +++ b/dlls/ntdll/directory.c
> > @@ -115,6 +115,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(file);
> >
> > /* just in case... */
> >   #undef VFAT_IOCTL_READDIR_BOTH
> > +#undef EXT2_IOC_GETFLAGS
> > +#undef EXT4_CASEFOLD_FL
> >
> > #ifdef linux
> >
> > @@ -130,6 +132,12 @@ typedef struct
> >   /* Define the VFAT ioctl to get both short and long file names */
> >   #define VFAT_IOCTL_READDIR_BOTH  _IOR('r', 1, KERNEL_DIRENT [2] )
> >
> > +/* Define the ext2 ioctl for handling extra attributes */
> > +#define EXT2_IOC_GETFLAGS _IOR('f', 1, long)
> > -
> >
> > +/* Case-insensitivity attribute */
> > +#define EXT4_CASEFOLD_FL 0x40000000
> > - #ifndef O_DIRECTORY
> > # define O_DIRECTORY 0200000 /* must be directory */
> > #endif
> >
> > @@ -1113,7 +1121,7 @@ static int get_dir_case_sensitivity_attr( const char *dir )
> > -
> >       get_dir_case_sensitivity_stat
> > -
> > - Checks if the volume containing the specified directory is case
> > -
> > - sensitive or not. Uses statfs(2) or statvfs(2).
> > -
> > - sensitive or not. Uses (f)statfs(2), statvfs(2), fstatat(2), or ioctl(2).
> > */
> > static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
> > {
> >
> > @@ -1165,14 +1173,23 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
> >       return FALSE;
> >
> > #elif defined(__linux__)
> > - BOOLEAN sens = FALSE;
> >  struct statfs stfs;
> >  struct stat st;
> > - char *cifile;
> > - int fd, flags;
> > -
> > - if ((fd = open( dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE )) == -1)
> > -
> >    return FALSE;
>
> I think returning TRUE in an error case here is the correct thing to do.
> TRUE is after all the default.

Yeah it seemed fishy to me as well but the previous code returned FALSE 
on error so I assumed it had some reason to do it. I'll do all the 
changes, thanks.