[PATCH 3/4] ntdll: Implement storing DOS attributes in NtSetInformationFile.

[Sebastian Lackner]

Hi Chip,

On 18.12.2014 21:23, cdavis5x at gmail.com wrote:
> 
>> On Dec 18, 2014, at 10:16 AM, Erich E. Hoover <erich.e.hoover at 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