Re: [PATCH 3/4] ntdll: Implement storing DOS attributes in NtSetInformationFile.
On Dec 18, 2014, at 10:16 AM, Erich E. Hoover <erich.e.hoover(a)gmail.com> wrote: This patch implements storing DOS attributes in the Samba format "user.DOSATTRIB" extended attribute using fsetxattr and fremovexattr. FILE_ATTRIBUTE_NORMAL is masked out since that attribute is unnecessary, and leaving it out avoids having to store an attribute for every file on the system. Only NORMAL?
There are quite a few DOS attributes. Here’s a list from <winnt.h>: #define FILE_ATTRIBUTE_READONLY 0x00000001 #define FILE_ATTRIBUTE_HIDDEN 0x00000002 #define FILE_ATTRIBUTE_SYSTEM 0x00000004 #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 #define FILE_ATTRIBUTE_ARCHIVE 0x00000020 #define FILE_ATTRIBUTE_DEVICE 0x00000040 #define FILE_ATTRIBUTE_NORMAL 0x00000080 #define FILE_ATTRIBUTE_TEMPORARY 0x00000100 #define FILE_ATTRIBUTE_SPARSE_FILE 0x00000200 #define FILE_ATTRIBUTE_REPARSE_POINT 0x00000400 #define FILE_ATTRIBUTE_COMPRESSED 0x00000800 #define FILE_ATTRIBUTE_OFFLINE 0x00001000 #define FILE_ATTRIBUTE_NOT_CONTENT_INDEXED 0x00002000 #define FILE_ATTRIBUTE_ENCRYPTED 0x00004000 (Yes, all of those are in Microsoft’s public headers. I checked.) Of those, these are the ones that Samba lets clients set: FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_OFFLINE and these are the ones that Samba returns: FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_DIRECTORY FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_SPARSE_FILE (Weird. They let people set OFFLINE, but never return it…) These are the ones that MSDN says Windows lets programs set with SetFileAttributes() (but I can’t vouch for MSDN’s accuracy, so this really needs to be demonstrated with a test): FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_NORMAL FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_TEMPORARY FILE_ATTRIBUTE_OFFLINE FILE_ATTRIBUTE_NOT_CONTENT_INDEXED and these are the ones that CreateFile() is supposed to let programs set: FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_NORMAL FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_TEMPORARY FILE_ATTRIBUTE_OFFLINE FILE_ATTRIBUTE_ENCRYPTED I definitely think we shouldn’t be letting people set the DEVICE attribute (this is, I suspect, the attribute you get from a device handle), or the REPARSE_POINT attribute (this is what Windows returns for e.g. symlinks and junctions); MSDN says that the SET_REPARSE_POINT FSCTL is supposed to be able to set this attribute, though. Samba also forcibly sets the DIRECTORY attribute according to whether the file is or is not actually a directory; we should do this, too. MSDN also says the COMPRESSED attribute is supposed to be set by the SET_COMPRESSION FSCTL, the ENCRYPTED attribute is supposed to be set by advapi32.EncryptFile() (which I suspect just makes a SET_ENCRYPTION FSCTL) or when the file is created, and the SPARSE_FILE attribute is supposed to be set by the SET_SPARSE FSCTL, so we should mask all those out too. (Or error out, depending on what native does; as always, the tests are your friends here.) Also, NORMAL is only supposed to be valid by itself, so if any attributes are set along with NORMAL, we may need to error out. We REALLY need tests for all this. ;) (We have some tests for setting file attributes, but not enough IMO.) Chip
Hi Chip, On 18.12.2014 21:23, cdavis5x(a)gmail.com wrote:
On Dec 18, 2014, at 10:16 AM, Erich E. Hoover <erich.e.hoover(a)gmail.com> wrote: This patch implements storing DOS attributes in the Samba format "user.DOSATTRIB" extended attribute using fsetxattr and fremovexattr. FILE_ATTRIBUTE_NORMAL is masked out since that attribute is unnecessary, and leaving it out avoids having to store an attribute for every file on the system. Only NORMAL?
There are quite a few DOS attributes. Here’s a list from <winnt.h>:
[...]
(Yes, all of those are in Microsoft’s public headers. I checked.)
Of those, these are the ones that Samba lets clients set:
FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_OFFLINE
and these are the ones that Samba returns:
FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_DIRECTORY FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_SPARSE_FILE
(Weird. They let people set OFFLINE, but never return it…)
Since Wine and Samba are two independent software packages it would be hard to keep all this in sync. The current solution of storing everything that might be useful for Samba seems to be easier than a hardcoded list.
These are the ones that MSDN says Windows lets programs set with SetFileAttributes() (but I can’t vouch for MSDN’s accuracy, so this really needs to be demonstrated with a test):
FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_NORMAL FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_TEMPORARY FILE_ATTRIBUTE_OFFLINE FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
and these are the ones that CreateFile() is supposed to let programs set:
FILE_ATTRIBUTE_READONLY FILE_ATTRIBUTE_HIDDEN FILE_ATTRIBUTE_SYSTEM FILE_ATTRIBUTE_NORMAL FILE_ATTRIBUTE_ARCHIVE FILE_ATTRIBUTE_TEMPORARY FILE_ATTRIBUTE_OFFLINE FILE_ATTRIBUTE_ENCRYPTED
I definitely think we shouldn’t be letting people set the DEVICE attribute (this is, I suspect, the attribute you get from a device handle), or the REPARSE_POINT attribute (this is what Windows returns for e.g. symlinks and junctions); MSDN says that the SET_REPARSE_POINT FSCTL is supposed to be able to set this attribute, though.
Samba also forcibly sets the DIRECTORY attribute according to whether the file is or is not actually a directory; we should do this, too.
MSDN also says the COMPRESSED attribute is supposed to be set by the SET_COMPRESSION FSCTL, the ENCRYPTED attribute is supposed to be set by advapi32.EncryptFile() (which I suspect just makes a SET_ENCRYPTION FSCTL) or when the file is created, and the SPARSE_FILE attribute is supposed to be set by the SET_SPARSE FSCTL, so we should mask all those out too. (Or error out, depending on what native does; as always, the tests are your friends here.)
Also, NORMAL is only supposed to be valid by itself, so if any attributes are set along with NORMAL, we may need to error out.
We REALLY need tests for all this. ;) (We have some tests for setting file attributes, but not enough IMO.)
If you ask me this is a separate issue, and not directly related to this patchset. Wine already has a lot of tests, and if there are situations where setting attributes should fail or attributes should be ignored, this means the bug is already present at the moment. Please note that this patchset stores all attributes, but does _not_ use all of them yet, which means this is not a big problem. This allows to add support for additional attributes like the ENCRYPTED attribute later. If there are specific combinations which are invalid, we have to filter them anyway when we parse the extended attributes, a check when setting them is not sufficient (but could be added at any time). Feel free to write a couple of tests for such situations. @Erich: While reviewing your patch I noticed that you used a buffer of size 10 - it might be better use at least 11, to be able to store "0x00000000\0". Moreover you should check if using ENOSYS instead of ENOTSUP makes the testbot error go away.
Chip
Regards, Sebastian
participants (2)
-
cdavis5x＠gmail.com -
Sebastian Lackner