[PATCH 0/4] MR951: ntdll: Add initial support for DOS file attributes (attempt 3)
This patch set is based upon [patches from Wine Staging](https://github.com/wine-staging/wine-staging/tree/master/patches/ntdll-DOS_A...) by Erich E. Hoover (@ehoover), and implements support for the `SYSTEM`, `HIDDEN` and `READONLY` DOS file attributes. These can implemented in various ways depending upon the capabilities of the operating system and the file system. However, this initial patch-set focusses on just one method: Samba-formatted Extended File Attributes. Modern filesystems generally support Extended File Attributes - auxiliary blobs of binary data that can be attached to a file. Samba uses the `user.DOSATTRIB` attribute to store DOS attribute information in the form of a hexadecimal value, and this patch-set implements a compatible mechanism. Support for additional storage methods to increase operating system and filesystem compatibility is planned for later patch submissions. This effort is part of a larger project I have been working on to get Msys2 and Cygwin working properly on Wine. The absence of DOS fule attribute support prevents one of the modes that Cygwin and Msys2 can use to emulate symbolic links from working correctly, which causes the Cygwin installer to fail: https://bugs.winehq.org/show_bug.cgi?id=15679 See Also * https://bugs.winehq.org/show_bug.cgi?id=9158 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951
From: "Erich E. Hoover" <erich.e.hoover(a)gmail.com> Co-authored-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- configure | 29 +++++++++++++++++++++++++++++ configure.ac | 4 ++++ dlls/ntdll/unix/file.c | 42 +++++++++++++++++++++++++++++++++++++++++- include/config.h.in | 6 ++++++ 4 files changed, 80 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 3088184aade..0ff9e72c951 100755 --- a/configure +++ b/configure @@ -9105,6 +9105,35 @@ then : fi + for ac_header in sys/xattr.h +do : + ac_fn_c_check_header_compile "$LINENO" "sys/xattr.h" "ac_cv_header_sys_xattr_h" "$ac_includes_default" +if test "x$ac_cv_header_sys_xattr_h" = xyes +then : + printf "%s\n" "#define HAVE_SYS_XATTR_H 1" >>confdefs.h + HAVE_XATTR=1 + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include <sys/xattr.h> +int +main (void) +{ +getxattr("", "", "", 0, 0, 0); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_compile "$LINENO" +then : + +printf "%s\n" "#define XATTR_ADDITIONAL_OPTIONS 1" >>confdefs.h + +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam conftest.$ac_ext +fi + +done + DLLFLAGS="" diff --git a/configure.ac b/configure.ac index 1e733962b75..8264c170b87 100644 --- a/configure.ac +++ b/configure.ac @@ -635,6 +635,10 @@ AC_CHECK_HEADERS([libprocstat.h],,, #include <sys/queue.h> #endif]) +AC_CHECK_HEADERS(sys/xattr.h, [HAVE_XATTR=1] + [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/xattr.h>]], [[getxattr("", "", "", 0, 0, 0);]])], + [AC_DEFINE(XATTR_ADDITIONAL_OPTIONS, 1, [Define if xattr functions take additional arguments (Mac OS X)])])]) + dnl **** Check for working dll **** AC_SUBST(DLLFLAGS,"") diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 7eb8dbe7ad4..c38b3003059 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -98,6 +98,9 @@ #ifdef HAVE_SYS_STATFS_H #include <sys/statfs.h> #endif +#ifdef HAVE_SYS_XATTR_H +#include <sys/xattr.h> +#endif #include <time.h> #include <unistd.h> @@ -167,6 +170,9 @@ typedef struct #define MAX_IGNORED_FILES 4 +#define SAMBA_XATTR_DOS_ATTRIB "user.DOSATTRIB" +#define XATTR_ATTRIBS_MASK (FILE_ATTRIBUTE_HIDDEN|FILE_ATTRIBUTE_SYSTEM) + struct file_identity { dev_t dev; @@ -355,6 +361,22 @@ NTSTATUS errno_to_status( int err ) } } + +static int xattr_get( const char *path, const char *name, void *value, size_t size ) +{ +#if defined(HAVE_SYS_XATTR_H) +#if defined(XATTR_ADDITIONAL_OPTIONS) + return getxattr( path, name, value, size, 0, 0 ); +#else + return getxattr( path, name, value, size ); +#endif +#else + errno = ENOSYS; + return -1; +#endif +} + + /* get space from the current directory data buffer, allocating a new one if necessary */ static void *get_dir_data_space( struct dir_data *data, unsigned int size ) { @@ -1451,6 +1473,18 @@ static inline ULONG get_file_attributes( const struct stat *st ) } +/* decode the xattr-stored DOS attributes */ +static int get_file_xattr( char *hexattr, int attrlen ) +{ + if (attrlen > 2 && hexattr[0] == '0' && hexattr[1] == 'x') + { + hexattr[attrlen] = 0; + return strtol( hexattr+2, NULL, 16 ) & XATTR_ATTRIBS_MASK; + } + return 0; +} + + static BOOL fd_is_mount_point( int fd, const struct stat *st ) { struct stat parent; @@ -1479,7 +1513,8 @@ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULON static int get_file_info( const char *path, struct stat *st, ULONG *attr ) { char *parent_path; - int ret; + char hexattr[11]; + int len, ret; *attr = 0; ret = lstat( path, st ); @@ -1505,6 +1540,11 @@ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) free( parent_path ); } *attr |= get_file_attributes( st ); + + len = xattr_get( path, SAMBA_XATTR_DOS_ATTRIB, hexattr, sizeof(hexattr)-1 ); + if (len != -1) + *attr |= get_file_xattr( hexattr, len ); + return ret; } diff --git a/include/config.h.in b/include/config.h.in index de1bf6c61eb..8f0c27bb265 100644 --- a/include/config.h.in +++ b/include/config.h.in @@ -679,6 +679,9 @@ /* Define to 1 if you have the <sys/vnode.h> header file. */ #undef HAVE_SYS_VNODE_H +/* Define to 1 if you have the <sys/xattr.h> header file. */ +#undef HAVE_SYS_XATTR_H + /* Define to 1 if you have the `tcdrain' function. */ #undef HAVE_TCDRAIN @@ -888,6 +891,9 @@ backward compatibility; new code need not use it. */ #undef STDC_HEADERS +/* Define if xattr functions take additional arguments (Mac OS X) */ +#undef XATTR_ADDITIONAL_OPTIONS + /* Define to 1 if the X Window System is missing or not being used. */ #undef X_DISPLAY_MISSING -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/951
On 9/28/22 07:35, Erich E. Hoover wrote:
+/* decode the xattr-stored DOS attributes */ +static int get_file_xattr( char *hexattr, int attrlen ) +{ + if (attrlen > 2 && hexattr[0] == '0' && hexattr[1] == 'x') + { + hexattr[attrlen] = 0; + return strtol( hexattr+2, NULL, 16 ) & XATTR_ATTRIBS_MASK; + } + return 0; +}
I don't know where this comes from (I guess Erich does?), but this doesn't seem to match what my Samba server does: whatsit(a)camazotz:~/vmshare$ attr -q -g DOSATTRIB test-file | xxd -g4 00000000: 00000400 04000000 11000000 21000000 ............!... 00000010: 00000000 00000000 c27bfa02 65c8d801 .........{..e... I'm not sure where the first four bytes come from (it may be worth asking the Samba folks about that), but the rest seems to be from the xattr_DosInfo structure at [1]. (As an interesting side note, it seems that Samba already has a system for storing extended attributes using xattr.) [1] https://git.samba.org/samba.git/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
On 9/28/22 07:35, Erich E. Hoover wrote:
@@ -1505,6 +1540,11 @@ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) free( parent_path ); } *attr |= get_file_attributes( st ); + + len = xattr_get( path, SAMBA_XATTR_DOS_ATTRIB, hexattr, sizeof(hexattr)-1 ); + if (len != -1) + *attr |= get_file_xattr( hexattr, len ); + return ret; }
Should we WARN or ERR if we get something other than ENODATA or ENOTSUP?
From: "Erich E. Hoover" <erich.e.hoover(a)gmail.com> Co-authored-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- dlls/ntdll/unix/file.c | 86 ++++++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index c38b3003059..61054808920 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -362,6 +362,36 @@ NTSTATUS errno_to_status( int err ) } +static int xattr_fremove( int filedes, const char *name ) +{ +#if defined(HAVE_SYS_XATTR_H) +#if defined(XATTR_ADDITIONAL_OPTIONS) + return fremovexattr( filedes, name, 0 ); +#else + return fremovexattr( filedes, name ); +#endif +#else + errno = ENOSYS; + return -1; +#endif +} + + +static int xattr_fset( int filedes, const char *name, const void *value, size_t size ) +{ +#if defined(HAVE_SYS_XATTR_H) +#if defined(XATTR_ADDITIONAL_OPTIONS) + return fsetxattr( filedes, name, value, size, 0, 0 ); +#else + return fsetxattr( filedes, name, value, size, 0 ); +#endif +#else + errno = ENOSYS; + return -1; +#endif +} + + static int xattr_get( const char *path, const char *name, void *value, size_t size ) { #if defined(HAVE_SYS_XATTR_H) @@ -1509,6 +1539,41 @@ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULON } +/* set the stat info and file attributes for a file (by file descriptor) */ +NTSTATUS fd_set_file_info( int fd, ULONG attr ) +{ + struct stat st; + + if (fstat( fd, &st ) == -1) return errno_to_status( errno ); + if (attr & FILE_ATTRIBUTE_READONLY) + { + if (S_ISDIR( st.st_mode)) + WARN("FILE_ATTRIBUTE_READONLY ignored for directory.\n"); + else + st.st_mode &= ~0222; /* clear write permission bits */ + } + else + { + /* add write permission only where we already have read permission */ + st.st_mode |= (0600 | ((st.st_mode & 044) >> 1)) & (~start_umask); + } + if (fchmod( fd, st.st_mode ) == -1) return errno_to_status( errno ); + + /* do not store everything, but keep everything Samba can use */ + attr &= ~FILE_ATTRIBUTE_NORMAL; + if (attr != 0) + { + char hexattr[11]; + int len = sprintf( hexattr, "0x%x", attr ); + xattr_fset( fd, SAMBA_XATTR_DOS_ATTRIB, hexattr, len ); + } + else + xattr_fremove( fd, SAMBA_XATTR_DOS_ATTRIB ); + + return STATUS_SUCCESS; +} + + /* get the stat info and file attributes for a file (by name) */ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) { @@ -4403,7 +4468,6 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, case FileBasicInformation: if (len >= sizeof(FILE_BASIC_INFORMATION)) { - struct stat st; const FILE_BASIC_INFORMATION *info = ptr; LARGE_INTEGER mtime, atime; @@ -4417,25 +4481,7 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, status = set_file_times( fd, &mtime, &atime ); if (status == STATUS_SUCCESS && info->FileAttributes) - { - if (fstat( fd, &st ) == -1) status = errno_to_status( errno ); - else - { - if (info->FileAttributes & FILE_ATTRIBUTE_READONLY) - { - if (S_ISDIR( st.st_mode)) - WARN("FILE_ATTRIBUTE_READONLY ignored for directory.\n"); - else - st.st_mode &= ~0222; /* clear write permission bits */ - } - else - { - /* add write permission only where we already have read permission */ - st.st_mode |= (0600 | ((st.st_mode & 044) >> 1)) & (~start_umask); - } - if (fchmod( fd, st.st_mode ) == -1) status = errno_to_status( errno ); - } - } + status = fd_set_file_info( fd, info->FileAttributes ); if (needs_close) close( fd ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/951
On 9/28/22 07:35, Erich E. Hoover wrote:
+/* set the stat info and file attributes for a file (by file descriptor) */ +NTSTATUS fd_set_file_info( int fd, ULONG attr ) +{ + /* do not store everything, but keep everything Samba can use */ + attr &= ~FILE_ATTRIBUTE_NORMAL; + if (attr != 0) + { + char hexattr[11]; + int len = sprintf( hexattr, "0x%x", attr ); + xattr_fset( fd, SAMBA_XATTR_DOS_ATTRIB, hexattr, len ); + } + else + xattr_fremove( fd, SAMBA_XATTR_DOS_ATTRIB ); + + return STATUS_SUCCESS; +}
Why do we apply XATTR_ATTRIBS_MASK when setting file attributes, but not getting them? Admittedly it's not quite clear to me whether to do one or the other, but the inconsistency is confusing. I will note however that this will presumably set FILE_ATTRIBUTE_DIRECTORY on directories, which probably isn't what we want. We may also want to mask out FILE_ATTRIBUTE_REPARSE_POINT if Windows isn't going to handle it correctly. I also notice that CreateFile() applies FILE_ATTRIBUTE_ARCHIVE to all files regardless of whether it was requested. Should we mask that out as well? Then again, I don't know if there's any real (space) cost to setting that on all Wine files, and maybe we should do it for correctness anyway.
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=9158 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=15679 Co-authored-by: Erich E. Hoover <erich.e.hoover(a)gmail.com> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- dlls/ntdll/tests/file.c | 8 ++++---- dlls/ntdll/unix/file.c | 23 ++++++++++++++++++++++- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index dd0061b13d8..d49d4d5c26f 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -1392,7 +1392,7 @@ static void test_file_basic_information(void) memset(&fbi, 0, sizeof(fbi)); res = pNtQueryInformationFile(h, &io, &fbi, sizeof fbi, FileBasicInformation); ok ( res == STATUS_SUCCESS, "can't get attributes\n"); - todo_wine ok ( (fbi.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_SYSTEM, "attribute %lx not FILE_ATTRIBUTE_SYSTEM\n", fbi.FileAttributes ); + ok ( (fbi.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_SYSTEM, "attribute %lx not FILE_ATTRIBUTE_SYSTEM\n", fbi.FileAttributes ); /* Then HIDDEN */ memset(&fbi, 0, sizeof(fbi)); @@ -1405,7 +1405,7 @@ static void test_file_basic_information(void) memset(&fbi, 0, sizeof(fbi)); res = pNtQueryInformationFile(h, &io, &fbi, sizeof fbi, FileBasicInformation); ok ( res == STATUS_SUCCESS, "can't get attributes\n"); - todo_wine ok ( (fbi.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_HIDDEN, "attribute %lx not FILE_ATTRIBUTE_HIDDEN\n", fbi.FileAttributes ); + ok ( (fbi.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_HIDDEN, "attribute %lx not FILE_ATTRIBUTE_HIDDEN\n", fbi.FileAttributes ); /* Check NORMAL last of all (to make sure we can clear attributes) */ memset(&fbi, 0, sizeof(fbi)); @@ -1462,7 +1462,7 @@ static void test_file_all_information(void) memset(&fai_buf.fai, 0, sizeof(fai_buf.fai)); res = pNtQueryInformationFile(h, &io, &fai_buf.fai, sizeof fai_buf, FileAllInformation); ok ( res == STATUS_SUCCESS, "can't get attributes, res %x\n", res); - todo_wine ok ( (fai_buf.fai.BasicInformation.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_SYSTEM, "attribute %lx not FILE_ATTRIBUTE_SYSTEM\n", fai_buf.fai.BasicInformation.FileAttributes ); + ok ( (fai_buf.fai.BasicInformation.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_SYSTEM, "attribute %lx not FILE_ATTRIBUTE_SYSTEM\n", fai_buf.fai.BasicInformation.FileAttributes ); /* Then HIDDEN */ memset(&fai_buf.fai.BasicInformation, 0, sizeof(fai_buf.fai.BasicInformation)); @@ -1475,7 +1475,7 @@ static void test_file_all_information(void) memset(&fai_buf.fai, 0, sizeof(fai_buf.fai)); res = pNtQueryInformationFile(h, &io, &fai_buf.fai, sizeof fai_buf, FileAllInformation); ok ( res == STATUS_SUCCESS, "can't get attributes\n"); - todo_wine ok ( (fai_buf.fai.BasicInformation.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_HIDDEN, "attribute %lx not FILE_ATTRIBUTE_HIDDEN\n", fai_buf.fai.BasicInformation.FileAttributes ); + ok ( (fai_buf.fai.BasicInformation.FileAttributes & attrib_mask) == FILE_ATTRIBUTE_HIDDEN, "attribute %lx not FILE_ATTRIBUTE_HIDDEN\n", fai_buf.fai.BasicInformation.FileAttributes ); /* Check NORMAL last of all (to make sure we can clear attributes) */ memset(&fai_buf.fai.BasicInformation, 0, sizeof(fai_buf.fai.BasicInformation)); diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 61054808920..cef1d871779 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -407,6 +407,21 @@ static int xattr_get( const char *path, const char *name, void *value, size_t si } +static int xattr_fget( int filedes, const char *name, void *value, size_t size ) +{ +#if defined(HAVE_SYS_XATTR_H) +#if defined(XATTR_ADDITIONAL_OPTIONS) + return fgetxattr( filedes, name, value, size, 0, 0 ); +#else + return fgetxattr( filedes, name, value, size ); +#endif +#else + errno = ENOSYS; + return -1; +#endif +} + + /* get space from the current directory data buffer, allocating a new one if necessary */ static void *get_dir_data_space( struct dir_data *data, unsigned int size ) { @@ -1526,7 +1541,8 @@ static BOOL fd_is_mount_point( int fd, const struct stat *st ) /* get the stat info and file attributes for a file (by file descriptor) */ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULONG *attr ) { - int ret; + char hexattr[11]; + int len, ret; *attr = 0; ret = fstat( fd, st ); @@ -1535,6 +1551,11 @@ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULON /* consider mount points to be reparse points (IO_REPARSE_TAG_MOUNT_POINT) */ if ((options & FILE_OPEN_REPARSE_POINT) && fd_is_mount_point( fd, st )) *attr |= FILE_ATTRIBUTE_REPARSE_POINT; + + len = xattr_fget( fd, SAMBA_XATTR_DOS_ATTRIB, hexattr, sizeof(hexattr)-1 ); + if (len != -1) + *attr |= get_file_xattr( hexattr, len ); + return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/951
From: "Erich E. Hoover" <erich.e.hoover(a)gmail.com> Co-authored-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- dlls/ntdll/tests/directory.c | 25 ++++++++++++------------ dlls/ntdll/unix/file.c | 37 ++++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index 2a5fedb4659..a5ea7900f8d 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -55,7 +55,6 @@ static NTSTATUS (WINAPI *pRtlWow64EnableFsRedirectionEx)( ULONG disable, ULONG * /* The attribute sets to test */ static struct testfile_s { - BOOL todo; /* set if it doesn't work on wine yet */ BOOL attr_done; /* set if attributes were tested for this file already */ const DWORD attr; /* desired attribute */ WCHAR name[20]; /* filename to use */ @@ -63,16 +62,16 @@ static struct testfile_s { const char *description; /* for error messages */ int nfound; /* How many were found (expect 1) */ } testfiles[] = { - { 0, 0, FILE_ATTRIBUTE_NORMAL, {'l','o','n','g','f','i','l','e','n','a','m','e','.','t','m','p'}, "normal" }, - { 0, 0, FILE_ATTRIBUTE_NORMAL, {'n','.','t','m','p',}, "normal" }, - { 1, 0, FILE_ATTRIBUTE_HIDDEN, {'h','.','t','m','p',}, "hidden" }, - { 1, 0, FILE_ATTRIBUTE_SYSTEM, {'s','.','t','m','p',}, "system" }, - { 0, 0, FILE_ATTRIBUTE_DIRECTORY, {'d','.','t','m','p',}, "directory" }, - { 0, 0, FILE_ATTRIBUTE_NORMAL, {0xe9,'a','.','t','m','p'}, "normal" }, - { 0, 0, FILE_ATTRIBUTE_NORMAL, {0xc9,'b','.','t','m','p'}, "normal" }, - { 0, 0, FILE_ATTRIBUTE_NORMAL, {'e','a','.','t','m','p'}, "normal" }, - { 0, 0, FILE_ATTRIBUTE_DIRECTORY, {'.'}, ". directory" }, - { 0, 0, FILE_ATTRIBUTE_DIRECTORY, {'.','.'}, ".. directory" } + { 0, FILE_ATTRIBUTE_NORMAL, {'l','o','n','g','f','i','l','e','n','a','m','e','.','t','m','p'}, "normal" }, + { 0, FILE_ATTRIBUTE_NORMAL, {'n','.','t','m','p',}, "normal" }, + { 0, FILE_ATTRIBUTE_HIDDEN, {'h','.','t','m','p',}, "hidden" }, + { 0, FILE_ATTRIBUTE_SYSTEM, {'s','.','t','m','p',}, "system" }, + { 0, FILE_ATTRIBUTE_DIRECTORY, {'d','.','t','m','p',}, "directory" }, + { 0, FILE_ATTRIBUTE_NORMAL, {0xe9,'a','.','t','m','p'}, "normal" }, + { 0, FILE_ATTRIBUTE_NORMAL, {0xc9,'b','.','t','m','p'}, "normal" }, + { 0, FILE_ATTRIBUTE_NORMAL, {'e','a','.','t','m','p'}, "normal" }, + { 0, FILE_ATTRIBUTE_DIRECTORY, {'.'}, ". directory" }, + { 0, FILE_ATTRIBUTE_DIRECTORY, {'.','.'}, ".. directory" } }; static const int test_dir_count = ARRAY_SIZE(testfiles); static const int max_test_dir_size = ARRAY_SIZE(testfiles) + 5; /* size of above plus some for .. etc */ @@ -162,8 +161,8 @@ static void tally_test_file(FILE_BOTH_DIRECTORY_INFORMATION *dir_info) if (namelen != len || memcmp(nameW, testfiles[i].name, len*sizeof(WCHAR))) continue; if (!testfiles[i].attr_done) { - todo_wine_if (testfiles[i].todo) - ok (attrib == (testfiles[i].attr & attribmask), "file %s: expected %s (%lx), got %lx (is your linux new enough?)\n", wine_dbgstr_w(testfiles[i].name), testfiles[i].description, testfiles[i].attr, attrib); + ok (attrib == (testfiles[i].attr & attribmask), "file %s: expected %s (%lx), got %lx\n", + wine_dbgstr_w(testfiles[i].name), testfiles[i].description, testfiles[i].attr, attrib); testfiles[i].attr_done = TRUE; } testfiles[i].nfound++; diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index cef1d871779..d73e3b0a22d 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -1560,6 +1560,21 @@ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULON } +static int fd_set_dos_attrib( int fd, ULONG attr ) +{ + /* do not store everything, but keep everything Samba can use */ + attr &= ~FILE_ATTRIBUTE_NORMAL; + if (attr != 0) + { + char hexattr[11]; + int len = sprintf( hexattr, "0x%x", attr ); + return xattr_fset( fd, SAMBA_XATTR_DOS_ATTRIB, hexattr, len ); + } + else + return xattr_fremove( fd, SAMBA_XATTR_DOS_ATTRIB ); +} + + /* set the stat info and file attributes for a file (by file descriptor) */ NTSTATUS fd_set_file_info( int fd, ULONG attr ) { @@ -1580,16 +1595,7 @@ NTSTATUS fd_set_file_info( int fd, ULONG attr ) } if (fchmod( fd, st.st_mode ) == -1) return errno_to_status( errno ); - /* do not store everything, but keep everything Samba can use */ - attr &= ~FILE_ATTRIBUTE_NORMAL; - if (attr != 0) - { - char hexattr[11]; - int len = sprintf( hexattr, "0x%x", attr ); - xattr_fset( fd, SAMBA_XATTR_DOS_ATTRIB, hexattr, len ); - } - else - xattr_fremove( fd, SAMBA_XATTR_DOS_ATTRIB ); + fd_set_dos_attrib( fd, attr ); return STATUS_SUCCESS; } @@ -3976,6 +3982,17 @@ NTSTATUS WINAPI NtCreateFile( HANDLE *handle, ACCESS_MASK access, OBJECT_ATTRIBU io->Information = FILE_OVERWRITTEN; break; } + + if (io->Information == FILE_CREATED) + { + int fd, needs_close; + + /* set any DOS extended attributes */ + if (!(status = server_get_unix_fd( *handle, 0, &fd, &needs_close, NULL, NULL ))) { + fd_set_dos_attrib( fd, attributes ); + if (needs_close) close( fd ); + } + } } else if (status == STATUS_TOO_MANY_OPENED_FILES) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/951
This merge request was approved by Joel Holdsworth. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951
On Wed Sep 28 19:26:04 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/28/22 07:35, Erich E. Hoover wrote:
+/* decode the xattr-stored DOS attributes */ +static int get_file_xattr( char *hexattr, int attrlen ) +{ + if (attrlen > 2 && hexattr[0] == '0' && hexattr[1] == 'x') + { + hexattr[attrlen] = 0; + return strtol( hexattr+2, NULL, 16 ) & XATTR_ATTRIBS_MASK; + } + return 0; +} I don't know where this comes from (I guess Erich does?), but this doesn't seem to match what my Samba server does: whatsit(a)camazotz:~/vmshare$ attr -q -g DOSATTRIB test-file | xxd -g4 00000000: 00000400 04000000 11000000 21000000 ............!... 00000010: 00000000 00000000 c27bfa02 65c8d801 .........{..e... I'm not sure where the first four bytes come from (it may be worth asking the Samba folks about that), but the rest seems to be from the xattr_DosInfo structure at [1]. (As an interesting side note, it seems that Samba already has a system for storing extended attributes using xattr.) [1] https://git.samba.org/samba.git/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
@zfigura That's because this patch was originally written against the [old Samba 3 format](https://git.samba.org/?p=samba.git;a=blob;f=source3/smbd/dosmode.c;h=555718bd83a02f2760365e3c83984003fc25326e;hb=c3632e4efc506a401a9d75c4d75b14a1a220caa2#l250), Samba 4 introduced a [new format and has made about 5 revisions to it](https://git.samba.org/?p=samba.git;a=blob;f=source3/smbd/dosmode.c#l236).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9723
On 9/28/22 17:33, Erich Hoover (@ehoover) wrote:
@zfigura That's because this patch was originally written against the [old Samba 3 format](https://git.samba.org/?p=samba.git;a=blob;f=source3/smbd/dosmode.c;h=555718b...), Samba 4 introduced a [new format and has made about 5 revisions to it](https://git.samba.org/?p=samba.git;a=blob;f=source3/smbd/dosmode.c#l236).
Thanks, that makes sense. I suppose it should be easy to detect whether the old or new format is in use, if indeed we care about supporting Samba 3. (It is 10 years old by now, but I wouldn't be surprised if there are still servers using it...)
On Wed Sep 28 20:42:41 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/28/22 07:35, Erich E. Hoover wrote:
@@ -1505,6 +1540,11 @@ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) free( parent_path ); } *attr |= get_file_attributes( st ); + + len = xattr_get( path, SAMBA_XATTR_DOS_ATTRIB, hexattr, sizeof(hexattr)-1 ); + if (len != -1) + *attr |= get_file_xattr( hexattr, len ); + return ret; }
Should we WARN or ERR if we get something other than ENODATA or ENOTSUP? ``` @zfigura I would suspect that if you got something else, it would probably be ERANGE and you would likely get it for every file... That said, I doubt it would hurt anything.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9724
On 9/28/22 17:37, Erich Hoover (@ehoover) wrote:
@zfigura I would suspect that if you got something else, it would probably be ERANGE and you would likely get it for every file... That said, I doubt it would hurt anything.
Yes, that's the rub, really :-) I basically downloaded these patches, tried to test them with my samba setup, found they silently didn't work, and discovered it was because we were getting ERANGE. As long as the format has a fixed (or capped) size, which seems to be true of both the Samba 3 and Samba 4 formats, I think it'd make sense to be noisy about ERANGE, since that implies there's a new format we can't parse. And if not, we should probably be calling it in a loop until we get the whole buffer. Either way, we should complain louder if there's data there that we can't parse.
Why do we apply XATTR_ATTRIBS_MASK when setting file attributes, but not getting them? Admittedly it's not quite clear to me whether to do one or the other, but the inconsistency is confusing.
You have that backwards, we store everything when setting (`FILE_ATTRIBUTE_NORMAL` is 0, the absence of other attributes, so `flags & ~0` gives everything) and mask out the values for Wine when reading (`XATTR_ATTRIBS_MASK` is for the attributes Wine will pay attention to).
I will note however that this will presumably set FILE_ATTRIBUTE_DIRECTORY on directories, which probably isn't what we want. We may also want to mask out FILE_ATTRIBUTE_REPARSE_POINT if Windows isn't going to handle it correctly.
I would suggest that, for correctness, we wish to store all attributes and on reading we mask out the values we need. So, we store `FILE_ATTRIBUTE_DIRECTORY` (or whatever else we have) when we create and we decide for ourselves whether it's a directory just like we do now.
I also notice that CreateFile() applies FILE_ATTRIBUTE_ARCHIVE to all files regardless of whether it was requested. Should we mask that out as well? Then again, I don't know if there's any real (space) cost to setting that on all Wine files, and maybe we should do it for correctness anyway.
I don't know why `CreateFile` does this, but if that's correct behavior then I would say we should store it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9726
I suppose it should be easy to detect whether the old or new format is in use, if indeed we care about supporting Samba 3. (It is 10 years old by now, but I wouldn't be surprised if there are still servers using it...)
At the time this was originally put together, it would have been odd to jump to supporting Samba 4. Even so, under the principle of "greatest compatibility" (Samba 4 still supports the Samba 3 format) I would argue we should probably start with the Samba 3 format and then add read-only support for the new format as a separate patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9729
I basically downloaded these patches, tried to test them with my samba setup, found they silently didn't work, and discovered it was because we were getting ERANGE.
If you create the hidden/system/whatever file on the Wine side then Samba should pick it up correctly the other direction.
As long as the format has a fixed (or capped) size, which seems to be true of both the Samba 3 and Samba 4 formats, I think it'd make sense to be noisy about ERANGE, since that implies there's a new format we can't parse.
I think that `FIXME_ONCE` might be a better choice here, as you will otherwise get this output for _everything_.
And if not, we should probably be calling it in a loop until we get the whole buffer. Either way, we should complain louder if there's data there that we can't parse.
The sizes are fixed and small, I would think that one read of the maximum supported size makes the most sense. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9730
On 9/28/22 18:24, Erich Hoover (@ehoover) wrote:
I basically downloaded these patches, tried to test them with my samba setup, found they silently didn't work, and discovered it was because we were getting ERANGE.
If you create the hidden/system/whatever file on the Wine side then Samba should pick it up correctly the other direction.
Indeed. Unfortunately I tested the broken direction first ;-)
As long as the format has a fixed (or capped) size, which seems to be true of both the Samba 3 and Samba 4 formats, I think it'd make sense to be noisy about ERANGE, since that implies there's a new format we can't parse.
I think that `FIXME_ONCE` might be a better choice here, as you will otherwise get this output for _everything_.
Sure, makes sense.
I don't know why `CreateFile` does this, but if that's correct behavior then I would say we should store it.
I think this stuff should be limited to "unusual" attributes, basically system+hidden. We don't want to be setting xattr on every single file. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/951#note_9734
participants (7)
-
Alexandre Julliard (＠julliard) -
Erich E. Hoover
-
Erich Hoover (＠ehoover)
-
Joel Holdsworth
-
Joel Holdsworth (＠jhol)
-
Zebediah Figura -
Zebediah Figura (＠zfigura)