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

[Gabriel Ivăncescu]

On 5/23/19 8:02 PM, André Hentschel wrote:
> Hi, just a quick review below
> 
> Am 22.05.19 um 19:32 schrieb Gabriel Ivăncescu:
>>   dlls/ntdll/directory.c | 66 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
>> index bbdbbe9..e5fa48d 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
> 
> why not this construct:
> #ifndef X
> #define X
> #endif
> like below:
> 

Hi André, thanks for the review.

Indeed, I was using the same construct as for the VFAT_IOCTL since it's 
also an ioctl, I assumed it was done that way for a reason. (perhaps it 
might be defined on non-linux systems?). Since this is also an ioctl I 
followed the same pattern, I think this one should be kept as-is until 
we find out the reason why the others are done this way.

>>   #ifndef O_DIRECTORY
>>   # define O_DIRECTORY 0200000 /* must be directory */
>>   #endif
>> @@ -977,6 +985,25 @@ static char *get_device_mount_point( dev_t dev )
>>   }
>>   
>>   
>> +/***********************************************************************
>> + *           get_dir_case_sensitivity_ioctl
>> + *
>> + * Checks if the specified directory is case sensitive or not. Uses ioctl(2).
>> + */
>> +static int get_dir_case_sensitivity_ioctl(int fd)
>> +{
>> +#define GET_DIR_CASE_SENSITIVITY_USE_FD 1
>> +#if defined(EXT2_IOC_GETFLAGS) && defined(EXT4_CASEFOLD_FL)
>> +    int flags;
>> +    if (ioctl(fd, EXT2_IOC_GETFLAGS, &flags) != -1 && (flags & EXT4_CASEFOLD_FL))
>> +        return FALSE;
>> +#else
>> +#undef GET_DIR_CASE_SENSITIVITY_USE_FD
>> +#define GET_DIR_CASE_SENSITIVITY_USE_FD 0
>> +#endif
> 
> I would define GET_DIR_CASE_SENSITIVITY_USE_FD at the top together with the EXT* defines it "depends" on.
> And this would look better:
> 
> #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
> 

Ah alright, I've done it that way to be able to be extended if in 
hypothetical future we'll also need fd for other checks, but using your 
style it would be trivial to extend it also, so I'll go with it.

>> +    return -1;
>> +}
>> +
>>   #if defined(HAVE_GETATTRLIST) && defined(ATTR_VOL_CAPABILITIES) && \
>>       defined(VOL_CAPABILITIES_FORMAT) && defined(VOL_CAP_FMT_CASE_SENSITIVE)
>>   
>> @@ -1115,12 +1142,16 @@ static int get_dir_case_sensitivity_attr( const char *dir )
>>    * Checks if the volume containing the specified directory is case
>>    * sensitive or not. Uses statfs(2) or statvfs(2).
> 
> Updating the comments above would be great for future readers
> 

Right, I hadn't noticed those were already wrong. Thanks for pointing it 
out.

>>    */
>> -static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
>> +static BOOLEAN get_dir_case_sensitivity_stat( const char *dir, int fd )
> 
> we only use fd if GET_DIR_CASE_SENSITIVITY_USE_FD==1, can we improve something here to avoid an unused parameter in the other case?
> 

An unused parameter doesn't generate a warning, however, now that I 
think about it, I can just test if fd is -1 instead of using 
GET_DIR_CASE_SENSITIVITY_USE_FD everywhere except when opening the file. 
(the compiler will fold it anyway so it doesn't matter)

It should make the code cleaner overall and also avoid unused param. :-)