Re: [PATCH v2 0/8] MR924: ntdll: Add support for DOS file attributes
On Mon Sep 26 19:51:22 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` Hello Joel, I haven't looked at these patches in detail, largely because there's still outstanding test failures, but I do have one high-level comment, which is that I'd recommend starting with the basic implementation (i.e. just the first four patches), and leave the rest for separate merge requests. Since merge requests can only be applied as a whole I think it makes sense to narrow this down to a more minimal set, and try to get one thing done at a time. ``` Hi Zebediah - thanks for taking a look.
I don't mind narrowinging the commit series down. This patch set is mainly trying to get these attribute flags working: * `FILE_ATTRIBUTE_HIDDEN` * `FILE_ATTRIBUTE_SYSTEM` * `FILE_ATTRIBUTE_READONLY` (to a lesser extent - because this is already quite well represented by UNIX access permissions) The trouble is that while we want to seamlessly integrate with the underlying filesystems, the availability of different methods varies between operating systems and filesystems. This version of the patch set offers three strategies, and I found a two more possible methods that should be added today ([1](https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html) [2](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/n...) ). The code attempts to store the flags by all available methods at once for a given file, and ORs the flags together from any available source, in the hopes we can deliver the most complete coverage to the user. However, because there is no fallback method, it is currenly not possible to guaruantee that the attributes will always be stored and retrieved correctly in all scenaries. If we only include the first four patches, the patch set will only support storage in Samba-formatted Extended File Attributes. If we unmark the associated test cases as `todo_wine`, the test will break on certain operating systems or certain mount flag choices. There is no support for Extended Attributes on FreeBSD or NetBSD, for example, and the MacOS Extended Atrribute APIs differ from the Linux ones. One approach is to fall back to using a `.dosattributes` directory when no other storage method is available, and store the attributes in files inside there. However, I didn't want to implement this functionality before having a conversation with the Wine project developers. I have been in conversation with @ehoover in the last couple of days concerning his work on Junction Points, which is a related topic to DOS attributes, and also causes issues for Msys2 in its own right. His strategy is to store the required metadata in a [`.REPARSE_POINT` hidden directory](https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-Junct...). There is also the storage of [NT extended-attribute (EA)](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntif...) values to consider, when we move toward a non-stub implementation of this feature. Perhaps it would be preferable to produce a design that can be encompass all these issues. Do we need to resolve this design question here and now? Or can we just proceed with Linux-only Extended Attributes in the short term? I think it would be good if we could get some feedback from the project maintainers on these questions. Do you have any thoughts? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/924#note_9292
On 9/26/22 15:25, Joel Holdsworth (@jhol) wrote:
The trouble is that while we want to seamlessly integrate with the underlying filesystems, the availability of different methods varies between operating systems and filesystems. This version of the patch set offers three strategies, and I found a two more possible methods that should be added today ([1](https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html) [2](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/n...) ).
The code attempts to store the flags by all available methods at once for a given file, and ORs the flags together from any available source, in the hopes we can deliver the most complete coverage to the user.
However, because there is no fallback method, it is currenly not possible to guaruantee that the attributes will always be stored and retrieved correctly in all scenaries.
If we only include the first four patches, the patch set will only support storage in Samba-formatted Extended File Attributes. If we unmark the associated test cases as `todo_wine`, the test will break on certain operating systems or certain mount flag choices. There is no support for Extended Attributes on FreeBSD or NetBSD, for example, and the MacOS Extended Atrribute APIs differ from the Linux ones.
Sure, I understand the concern. That said, currently they're broken on *all* platforms, and so I don't think it's necessarily a problem to effectively fix them on one platform at a time.
One approach is to fall back to using a `.dosattributes` directory when no other storage method is available, and store the attributes in files inside there. However, I didn't want to implement this functionality before having a conversation with the Wine project developers.
That's a good question, and one I don't think I have enough knowledge to answer. My gut is that if extended attributes are "universal enough" (and usable enough), and we can easily implement DOS attributes over them, modulo slight differences in the API, then there isn't really any need to implement something truly portable. I could be wrong about this, though. Regardless I think now is a good time to have such a conversation :-)
There is also the storage of [NT extended-attribute (EA)](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntif...) values to consider, when we move toward a non-stub implementation of this feature.
Perhaps it would be preferable to produce a design that can be encompass all these issues.
I don't have a good read on this, personally. I suppose it at least partly depends on whether NT extended attributes are used by any program. I'm at least not aware of any usage?
Do we need to resolve this design question here and now? Or can we just proceed with Linux-only Extended Attributes in the short term?
I think working out the design completely before submitting any patches would probably be a good idea.
One practical question. How do I make the test case `todo_wine` in some cases, an not in others? Do I just have to comment out the test? Or is there a trick to indicate that inconsistency between plaforms is expected?
With respect to different platforms that Wine may be run on, that's not something we've as yet really concerned ourself with. Of course, that's largely because nobody runs the tests on anything but Linux, but regardless, I don't think it's going to be a problem if tests fail temporarily on other platforms. I would place todo_wine annotations according to the Linux test failures (and specifically the gitlab CI test failures, not that I expect it to matter).
participants (2)
-
Joel Holdsworth (＠jhol) -
Zebediah Figura