Re: [PATCH 1/4] ntdll: Implement retrieving DOS attributes in NtQueryInformationFile (try 2).
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
Currently only Linux extended attributes are supported, when they are not available at compile time the ports implementation will fall back to a "not-supported" return and any such attributes will be ignored. It is necessary to place this feature in ports so that Mac and FreeBSD can be supported separately, as the semantics of these APIs are quite different on such systems (patches to come after this initial implementation).
That doesn't mean they should go into ports, especially not as such non-standard functions. Unless you really have to use them all over the place, they should remain in ntdll.
On Fri, Dec 19, 2014 at 2:52 AM, Alexandre Julliard julliard@winehq.org wrote:
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
Currently only Linux extended attributes are supported, when they are not available at compile time the ports implementation will fall back to a "not-supported" return and any such attributes will be ignored. It is necessary to place this feature in ports so that Mac and FreeBSD can be supported separately, as the semantics of these APIs are quite different on such systems (patches to come after this initial implementation).
That doesn't mean they should go into ports, especially not as such non-standard functions. Unless you really have to use them all over the place, they should remain in ntdll.
Well, this was our logic: *) We suspect that you may want patch 4 (108301) implemented in the wineserver so that attributes are stored like they are for fstat() on file creation (to prevent problems with lockfiles). *) We need extended attribute support in the wineserver to implement security descriptors/ACLs (which, unfortunately, cannot be done in ntdll). *) You may want to expose extended attributes as a winelib convenience feature for porting your customer’s applications.
We could also move all the file information gathering to the wineserver, but we suspected that the overhead in doing that was not something that you’d be a huge fan of.
I’m very open to changing the interface, I would have preferred to use the standard Linux interface - but Mac OS X screws that all up by having a different number of parameters for the same functions. We could use the BSD or Solaris interface if you prefer, but they are quite a bit different than what most people would expect (if they’re familiar with the Linux/OSX interface).
Best, Erich
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
Well, this was our logic: *) We suspect that you may want patch 4 (108301) implemented in the wineserver so that attributes are stored like they are for fstat() on file creation (to prevent problems with lockfiles). *) We need extended attribute support in the wineserver to implement security descriptors/ACLs (which, unfortunately, cannot be done in ntdll). *) You may want to expose extended attributes as a winelib convenience feature for porting your customer’s applications.
We could also move all the file information gathering to the wineserver, but we suspected that the overhead in doing that was not something that you’d be a huge fan of.
I’m very open to changing the interface, I would have preferred to use the standard Linux interface - but Mac OS X screws that all up by having a different number of parameters for the same functions. We could use the BSD or Solaris interface if you prefer, but they are quite a bit different than what most people would expect (if they’re familiar with the Linux/OSX interface).
Depending on the size of the functions, copying them into the server may be preferable. In any case it would be a good idea to submit the rest of the patches so we can see where this is going.
On Fri, Dec 19, 2014 at 10:40 AM, Alexandre Julliard julliard@winehq.org wrote:
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
... Depending on the size of the functions, copying them into the server may be preferable. In any case it would be a good idea to submit the rest of the patches so we can see where this is going.
For the rest of the DOS Attributes features there's only a couple more relevant patches ( https://github.com/wine-compholio/wine-staging/tree/master/patches/ntdll-DOS... ): Mac OS X) https://raw.githubusercontent.com/wine-compholio/wine-staging/master/patches... FreeBSD) https://raw.githubusercontent.com/wine-compholio/wine-staging/master/patches...
For the security descriptor patches we have a couple separate patchsets toward the full implementation (8 more patches): Bug #33576) storage/retrieval of SDs/ACLs (6 patches): https://github.com/wine-compholio/wine-staging/tree/master/patches/server-St... Bug #34406) inheritance of SDs/ACLs (2 patches): https://github.com/wine-compholio/wine-staging/tree/master/patches/server-In...
If you prefer i can submit them to wine-patches, but i figured that people probably don't want the spam.
Best, Erich
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
On Fri, Dec 19, 2014 at 10:40 AM, Alexandre Julliard julliard@winehq.org wrote:
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
... Depending on the size of the functions, copying them into the server may be preferable. In any case it would be a good idea to submit the rest of the patches so we can see where this is going.
For the rest of the DOS Attributes features there's only a couple more relevant patches ( https://github.com/wine-compholio/wine-staging/tree/master/patches/ntdll-DOS... ): Mac OS X) https://raw.githubusercontent.com/wine-compholio/wine-staging/master/patches... FreeBSD) https://raw.githubusercontent.com/wine-compholio/wine-staging/master/patches...
For the security descriptor patches we have a couple separate patchsets toward the full implementation (8 more patches): Bug #33576) storage/retrieval of SDs/ACLs (6 patches): https://github.com/wine-compholio/wine-staging/tree/master/patches/server-St... Bug #34406) inheritance of SDs/ACLs (2 patches): https://github.com/wine-compholio/wine-staging/tree/master/patches/server-In...
If you prefer i can submit them to wine-patches, but i figured that people probably don't want the spam.
Submitting them would be better, reading patches in a browser isn't very convenient. But from a quick look it seems that many parts will need more work, so I don't think you should be defining libport functions at this point.
On Fri, Dec 19, 2014 at 11:13 AM, Alexandre Julliard julliard@winehq.org wrote:
... I don't think you should be defining libport functions at this point.
I’m really hesitant to make such a change, the proper location for this code is somewhere that can be shared by the wineserver and ntdll. We’ve been field testing these patches for over a year and, to the best of our knowledge, any odd complexity in the code is to combat quirky behavior that we’ve seen as a result of user feedback.
This is actually the reason we implemented DOS Attribute support at all, we wanted to reduce the size of the patches for SD/ACL support to a more manageable size so that the patches would be easier to review. Adding this infrastructure isn’t just important for the SD/ACL patches, it provides compatibility with other software that needs to talk to Windows clients (Samba) and gives us the ability to save state information that we need to fix a variety of Wine bugs.
If moving xattr.c from ports to ntdll is the only thing you would like to have changed, I would be willing to do that, but please review the rest of the patches in this series first. It’s getting frustrating to send these patches over and over with only a little bit of feedback each time. A lot of time has passed since my first try and a lot of people are impatiently waiting to see these issues get fixed.
Best, Erich
"Erich E. Hoover" erich.e.hoover@gmail.com writes:
On Fri, Dec 19, 2014 at 11:13 AM, Alexandre Julliard julliard@winehq.org wrote:
... I don't think you should be defining libport functions at this point.
I’m really hesitant to make such a change, the proper location for this code is somewhere that can be shared by the wineserver and ntdll. We’ve been field testing these patches for over a year and, to the best of our knowledge, any odd complexity in the code is to combat quirky behavior that we’ve seen as a result of user feedback.
This is actually the reason we implemented DOS Attribute support at all, we wanted to reduce the size of the patches for SD/ACL support to a more manageable size so that the patches would be easier to review.
TBH I'm not convinced that adding a custom xattr that no other application recognizes is the right way to do ACL support, as opposed to for instance mapping to Unix ACLs.
There may be good reasons for doing things the way you do, but if so you have to present convincing arguments. Testing patches in the staging tree is of course a good thing, but it's not a substitute for a proper design. Particularly for things that are stored on disk, getting it right from the beginning is very important, since you can't easily change it later.
-
Alexandre Julliard -
Erich E. Hoover