This adds support for checking the case-insensitive attribute on ext4 with newer kernels so that Wine can rely on it for performance.
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 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.
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, this will unfortunately involve a bit more syscalls now, because it has to open() and close() the fd, but it shouldn't be too much of a problem. (the fstatfs and fstatat make it less impactful somewhat, so they won't have to lookup the directory again, hopefully mitigating some of it)
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
v3: Clean up the code and don't have unused parameters in any case.
GET_DIR_CASE_SENSITIVITY_USE_FD is used so it can be as generic as possible for fast tweaking and future expansion on other platforms, if they get something similar.
As it is now, on Linux, we always set the EXT4_CASEFOLD_FL flag ourselves. 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, to keep the code smaller. I didn't remove it because I didn't want to "hardcode" this assumption, so please let me know if that should be done or not.
dlls/ntdll/directory.c | 73 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 8 deletions(-)
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..354c00f 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,12 +132,25 @@ 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
#endif /* linux */
+/* 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 + #define IS_OPTION_TRUE(ch) ((ch) == 'y' || (ch) == 'Y' || (ch) == 't' || (ch) == 'T' || (ch) == '1') #define IS_SEPARATOR(ch) ((ch) == '\' || (ch) == '/')
@@ -1109,18 +1124,35 @@ static int get_dir_case_sensitivity_attr( const char *dir ) } #endif
+/*********************************************************************** + * 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) +{ +#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; +#endif + return -1; +} + /*********************************************************************** * 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), (f)statvfs(2), or fstatat(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 (statfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statfs( dir, &stfs ) == -1) return FALSE; + /* 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 +1189,9 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) #elif defined(__NetBSD__) struct statvfs stfs;
- if (statvfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatvfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statvfs( dir, &stfs ) == -1) return FALSE; + /* Only assume CIOPFS is case insensitive. */ if (strcmp( stfs.f_fstypename, "fusefs" ) || strncmp( stfs.f_mntfromname, "ciopfs", 5 )) @@ -1170,7 +1204,9 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) char *cifile;
/* Only assume CIOPFS is case insensitive. */ - if (statfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statfs( dir, &stfs ) == -1) return FALSE; + 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 +1216,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 (fd != -1) + { + 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 ); @@ -1201,16 +1244,30 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) * get_dir_case_sensitivity * * Checks if the volume containing the specified directory is case - * sensitive or not. Uses statfs(2) or statvfs(2). + * sensitive or not. Uses multiple methods, depending on platform. */ 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 (fd != -1) close(fd); + return case_sensitive; }
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu gabrielopcode@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..ae12105 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 * @@ -1162,6 +1206,7 @@ static int setup_config_dir(void) /* create the drive symlinks */
mkdir( "drive_c", 0777 ); + set_case_insensitive( "drive_c" ); symlink( "../drive_c", "dosdevices/c:" ); symlink( "/", "dosdevices/z:" );
On Fri, May 24, 2019 at 03:09:35PM +0300, Gabriel Ivăncescu wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu gabrielopcode@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..ae12105 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
Again, let's just include the header.
/* 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@@ -1162,6 +1206,7 @@ static int setup_config_dir(void) /* create the drive symlinks */
mkdir( "drive_c", 0777 );
- set_case_insensitive( "drive_c" ); symlink( "../drive_c", "dosdevices/c:" ); symlink( "/", "dosdevices/z:" );
I can confirm that this does correctly set the casefold flag (and yes, child directories also inherit it). Whether we want to do it is another question.
Huw.
On Fri, 7 Jun 2019 at 14:20, Huw Davies huw@codeweavers.com wrote:
I can confirm that this does correctly set the casefold flag (and yes, child directories also inherit it). Whether we want to do it is another question.
I'm having a hard time coming up with reasons not to, but that may just be lack of imagination on my part.
On Fri, Jun 07, 2019 at 02:30:24PM +0430, Henri Verbeet wrote:
On Fri, 7 Jun 2019 at 14:20, Huw Davies huw@codeweavers.com wrote:
I can confirm that this does correctly set the casefold flag (and yes, child directories also inherit it). Whether we want to do it is another question.
I'm having a hard time coming up with reasons not to, but that may just be lack of imagination on my part.
Not related to the decision, but I should have noted that one needs to have the casefold feature set on the filesystem for one to be able to set the casefold flag on a directory. That needs to be done by mke2fs, so we'll probably want distros to add it to /etc/mke2fs.conf .
Huw.
Henri Verbeet hverbeet@gmail.com writes:
On Fri, 7 Jun 2019 at 14:20, Huw Davies huw@codeweavers.com wrote:
I can confirm that this does correctly set the casefold flag (and yes, child directories also inherit it). Whether we want to do it is another question.
I'm having a hard time coming up with reasons not to, but that may just be lack of imagination on my part.
We may want to do that, but I'm hoping that some people would enable it manually and run with this for a while first. This way we can figure out if there are any reasons not to do it.
On 6/9/19 8:47 PM, Alexandre Julliard wrote:
Henri Verbeet hverbeet@gmail.com writes:
On Fri, 7 Jun 2019 at 14:20, Huw Davies huw@codeweavers.com wrote:
I can confirm that this does correctly set the casefold flag (and yes, child directories also inherit it). Whether we want to do it is another question.
I'm having a hard time coming up with reasons not to, but that may just be lack of imagination on my part.
We may want to do that, but I'm hoping that some people would enable it manually and run with this for a while first. This way we can figure out if there are any reasons not to do it.
In that case, I think the second patch at least should go into staging so people can run with it. That's because setting this flag manually after a prefix was already created is a bit of a pain since the directory has to be empty.
On Fri, May 24, 2019 at 03:09:34PM +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.
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 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.
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, this will unfortunately involve a bit more syscalls now, because it has to open() and close() the fd, but it shouldn't be too much of a problem. (the fstatfs and fstatat make it less impactful somewhat, so they won't have to lookup the directory again, hopefully mitigating some of it)
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
v3: Clean up the code and don't have unused parameters in any case.
GET_DIR_CASE_SENSITIVITY_USE_FD is used so it can be as generic as possible for fast tweaking and future expansion on other platforms, if they get something similar.
As it is now, on Linux, we always set the EXT4_CASEFOLD_FL flag ourselves. 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, to keep the code smaller. I didn't remove it because I didn't want to "hardcode" this assumption, so please let me know if that should be done or not.
dlls/ntdll/directory.c | 73 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 8 deletions(-)
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..354c00f 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,12 +132,25 @@ 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
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.
+/* 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.
- {
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 (fd != -1) close(fd);
- return case_sensitive;
}
FWIW I'm not too convinced about the name/fd thing, but it now doesn't look too ugly, so I guess it's ok.
Huw.
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?
+/* 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?
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.
On 6/7/19 2:53 PM, Huw Davies wrote:
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.
Just to be clear, you want me to include this header, right? And add a configure check for it?
On Fri, Jun 07, 2019 at 03:26:51PM +0300, Gabriel Ivăncescu wrote:
On 6/7/19 2:53 PM, Huw Davies wrote:
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.
Just to be clear, you want me to include this header, right? And add a configure check for it?
Yes, although Alexandre may prefer it the way you have it already. It's a trade off between adding another (small) build dependency vs adding a few #defines.
Huw.
-
Alexandre Julliard -
Gabriel Ivăncescu -
Henri Verbeet -
Huw Davies