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

[André Hentschel]

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:

>  #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

> +    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

>   */
> -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?

>  {
>  #if defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
>      struct statfs stfs;
>  
> +#if GET_DIR_CASE_SENSITIVITY_USE_FD
> +    if (fstatfs( fd, &stfs ) == -1) return FALSE;
> +#else
>      if (statfs( dir, &stfs ) == -1) return FALSE;
> +#endif
>      /* Assume these file systems are always case insensitive on Mac OS.
>       * For FreeBSD, only assume CIOPFS is case insensitive (AFAIK, Mac OS
>       * is the only UNIX that supports case-insensitive lookup).
> @@ -1157,7 +1188,11 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
>  #elif defined(__NetBSD__)
>      struct statvfs stfs;
>  
> +#if GET_DIR_CASE_SENSITIVITY_USE_FD
> +    if (fstatvfs( fd, &stfs ) == -1) return FALSE;
> +#else
>      if (statvfs( dir, &stfs ) == -1) return FALSE;
> +#endif
>      /* Only assume CIOPFS is case insensitive. */
>      if (strcmp( stfs.f_fstypename, "fusefs" ) ||
>          strncmp( stfs.f_mntfromname, "ciopfs", 5 ))
> @@ -1170,7 +1205,11 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
>      char *cifile;
>  
>      /* Only assume CIOPFS is case insensitive. */
> +#if GET_DIR_CASE_SENSITIVITY_USE_FD
> +    if (fstatfs( fd, &stfs ) == -1) return FALSE;
> +#else
>      if (statfs( dir, &stfs ) == -1) return FALSE;
> +#endif
>      if (stfs.f_type != 0x65735546 /* FUSE_SUPER_MAGIC */)
>          return TRUE;
>      /* Normally, we'd have to parse the mtab to find out exactly what
> @@ -1180,6 +1219,13 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
>       * This will break if somebody puts a file named ".ciopfs" in a non-
>       * CIOPFS directory.
>       */
> +    if (GET_DIR_CASE_SENSITIVITY_USE_FD)
> +    {
> +        if (fstatat( fd, ".ciopfs", &st, AT_NO_AUTOMOUNT ) == 0)
> +            return FALSE;
> +        return TRUE;
> +    }
> +
>      cifile = RtlAllocateHeap( GetProcessHeap(), 0, strlen( dir )+sizeof("/.ciopfs") );
>      if (!cifile) return TRUE;
>      strcpy( cifile, dir );
> @@ -1205,12 +1251,26 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir )
>   */
>  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 ((fd = open(dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE)) == -1)
> +            return TRUE;
> +        if ((case_sensitive = get_dir_case_sensitivity_ioctl(fd)) != -1)
> +            goto end;
> +    }
> +    case_sensitive = get_dir_case_sensitivity_stat(dir, fd);
> +
> +end:
> +    if (GET_DIR_CASE_SENSITIVITY_USE_FD) close(fd);
> +    return case_sensitive;
>  }
>  
>  
>