[PATCH v3 1/2] ntdll/directory: Add support for EXT4 case folding per directory

[Huw Davies]

On Fri, Jun 07, 2019 at 02:22:14PM +0300, Gabriel Ivăncescu wrote:
> Hi Huw,
> 
> On 6/7/19 12:47 PM, Huw Davies wrote:
> > On Fri, May 24, 2019 at 03:09:34PM +0300, Gabriel Ivăncescu wrote:
> > 
> > I'm not sure we need to define these ourselves, we can
> > just pull them from the headers.  I realise that you're
> > copying what was done with VFAT_IOCTL_READDIR_BOTH and
> > I don't know why that was done that way, but unless
> > there's a reason to do this, let's not add more.
> > 
> 
> EXT2_IOC_GETFLAGS seems to be defined in linux/ext2_fs.h. The problem is
> that I don't seem to have that header neither on my machine or build
> environments. I'm guessing it's part of some different dependency. (of
> course, I can use the ioctl manually if I define it myself)
> 
> It's probably the reason VFAT_IOCTL_READDIR_BOTH was defined like that as
> well, I'm guessing most people or distros don't have these headers with just
> Wine's dependencies.
> 
> Should I still import it from header or leave it as is?

Ah right, the kernel doesn't export them.
They are in ext2fs/ext2_fs.h (from e2fslibs-dev on Ubuntu) which
doesn't seem like too much of a burden.

> > > +/* Use a file descriptor for the case-sensitivity check if we have to */
> > > +#if defined(EXT2_IOC_GETFLAGS) && defined(EXT4_CASEFOLD_FL)
> > > +#define GET_DIR_CASE_SENSITIVITY_USE_FD 1
> > > +#else
> > > +#define GET_DIR_CASE_SENSITIVITY_USE_FD 0
> > > +#endif
> > 
> > This is now just used in one place, so get rid of it use the
> > preprocessor condition at the relevant place below.
> > 
> > >   static BOOLEAN get_dir_case_sensitivity( const char *dir )
> > >   {
> > > +    int case_sensitive, fd = -1;
> > > +
> > >   #if defined(HAVE_GETATTRLIST) && defined(ATTR_VOL_CAPABILITIES) && \
> > >       defined(VOL_CAPABILITIES_FORMAT) && defined(VOL_CAP_FMT_CASE_SENSITIVE)
> > > -    int case_sensitive = get_dir_case_sensitivity_attr( dir );
> > > +    case_sensitive = get_dir_case_sensitivity_attr( dir );
> > >       if (case_sensitive != -1) return case_sensitive;
> > >   #endif
> > > -    return get_dir_case_sensitivity_stat( dir );
> > > +
> > > +    if (GET_DIR_CASE_SENSITIVITY_USE_FD)
> > 
> > #if defined(EXT2_IOC_GETFLAGS) && defined(EXT4_CASEFOLD_FL)
> > 
> > No need for the if block.
> > 
> 
> The problem is that the preprocessor would eliminate the code from parsing,
> so when it's not defined, there will be warnings about unused variables or
> unused labels (the end label). That's why I used an if statement instead.
> 
> I can of course do something like:
> 
> #if !defined(EXT2_IOC_GETFLAGS) || !defined(EXT4_CASEFOLD_FL)
> if (0)
> #endif
> {
>     ...
> }
> 
> but I'm not sure that's preferable since it looks ugly.
> 
> 
> An alternative would be to use a helper function to open the fd, and in case
> we don't have the #defines, we just return -1 from it. Would that be
> suitable?

I don't see why you'd have unused variables.  The label will be
though, so since there are only two lines after the end label, I'd
probably just duplicate them inside the #if block.
get_dir_case_sensitivity_ioctl() will also be unused so that will have
to be in an ifdef block.

And to make things simpler, I think it's safe to assume that if
we have EXT4_CASEFOLD_FL then we'll have EXT2_IOC_[GS]ETFLAGS
so just test for the former (here and in any other places).

Huw.