[PATCH v2 1/2] ntdll/directory: Add support for EXT4 case folding per directory
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v2: Fix mistake with fstatat return value. This patch just adds support for the case-insensitive attribute on ext4 with newer kernels -- so it should be safe for upstream (barring stylistic and other changes). It has some conditional processing for perf reasons. 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 name (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* use a file descriptor, then use it (e.g. fstatfs) everywhere. However, to avoid a perf regression or downgrade on systems that don't support the EXT2_IOC_GETFLAGS ioctl (e.g. MacOS, FreeBSD), we continue using statfs and the like directly, this shaves off two syscalls (open/close). But in the case the EXT4_CASEFOLD_FL is not supported on Linux (i.e. on current kernels) or the directory doesn't have it (think of dirs outside drive_c), this will unfortunately involve a bit more syscalls now, because it has to open() and close() the fd. I'd really like if the fstatfs and fstatat can stay if we do use the fd, to somehow make it less impactful on current systems, so they won't have to lookup the directory again. So that's basically why the GET_DIR_CASE_SENSITIVITY_USE_FD constant is there. One last thing: as it is now, on Linux, we always set the EXT4_CASEFOLD_FL flag ourselves since it does no harm even if it's not supported. Thus, I can of course remove the .ciopfs old code and keep just the fstatat() call by assuming we always use fd on Linux, if you prefer it that way. I didn't remove it because I didn't want to "hardcode" this assumption, so I'm waiting for feedback. 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 + #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 + 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). */ -static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) +static BOOLEAN get_dir_case_sensitivity_stat( const char *dir, int fd ) { #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; } -- 2.21.0
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This marks the 'drive_c' directory as case-insensitive for filesystems that support it, when it is created on new prefixes (because it has to be empty anyway). The macro constant definitions were made similar in style to those in ntdll/directory. dlls/ntdll/server.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 094b530..0264ab2 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -51,6 +51,12 @@ #ifdef HAVE_SYS_MMAN_H #include <sys/mman.h> #endif +#ifdef HAVE_SYS_IOCTL_H +#include <sys/ioctl.h> +#endif +#ifdef HAVE_LINUX_IOCTL_H +#include <linux/ioctl.h> +#endif #ifdef HAVE_SYS_PRCTL_H # include <sys/prctl.h> #endif @@ -84,6 +90,22 @@ WINE_DEFAULT_DEBUG_CHANNEL(server); +/* just in case... */ +#undef EXT2_IOC_GETFLAGS +#undef EXT2_IOC_SETFLAGS +#undef EXT4_CASEFOLD_FL + +#ifdef __linux__ + +/* Define the ext2 ioctls for handling extra attributes */ +#define EXT2_IOC_GETFLAGS _IOR('f', 1, long) +#define EXT2_IOC_SETFLAGS _IOW('f', 2, long) + +/* Case-insensitivity attribute */ +#define EXT4_CASEFOLD_FL 0x40000000 + +#endif + /* Some versions of glibc don't define this */ #ifndef SCM_RIGHTS #define SCM_RIGHTS 1 @@ -1119,6 +1141,28 @@ static void start_server(void) } +/*********************************************************************** + * set_case_insensitive + * + * Make the supplied directory case insensitive, if available. + */ +static void set_case_insensitive(const char *dir) +{ +#if defined(EXT2_IOC_GETFLAGS) && defined(EXT2_IOC_SETFLAGS) && defined(EXT4_CASEFOLD_FL) + int flags, fd; + + if ((fd = open(dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE)) == -1) + return; + if (ioctl(fd, EXT2_IOC_GETFLAGS, &flags) != -1 && !(flags & EXT4_CASEFOLD_FL)) + { + flags |= EXT4_CASEFOLD_FL; + ioctl(fd, EXT2_IOC_SETFLAGS, &flags); + } + close(fd); +#endif +} + + /*********************************************************************** * setup_config_dir * @@ -1164,6 +1208,7 @@ static int setup_config_dir(void) mkdir( "drive_c", 0777 ); symlink( "../drive_c", "dosdevices/c:" ); symlink( "/", "dosdevices/z:" ); + set_case_insensitive( "drive_c" ); done: if (fd_cwd == -1) fd_cwd = open( "dosdevices/c:", O_RDONLY ); -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=52705 Your paranoid android. === debian9 (32 bit report) === ntdll: om.c:2116: Test failed: got 99 om.c:2131: Test failed: got 99 om.c:2131: Test failed: got 99
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=52704 Your paranoid android. === debian9 (32 bit report) === ntdll: om.c:2116: Test failed: got 99 om.c:2131: Test failed: got 99 om.c:2131: Test failed: got 99 om.c:2131: Test failed: got 98
On 5/22/19 8:36 PM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=52704
Your paranoid android.
=== debian9 (32 bit report) ===
ntdll: om.c:2116: Test failed: got 99 om.c:2131: Test failed: got 99 om.c:2131: Test failed: got 99 om.c:2131: Test failed: got 98
Ok, these look like hiccups to me. They're failures with WaitOnAddress, and these patches aren't doing anything remotely close to that, so is very likely unrelated.
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; }
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. :-)
participants (3)
-
André Hentschel -
Gabriel Ivăncescu -
Marvin