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.