Re: [2/2] ntdll: Implement FILE_ATTRIBUTE_HIDDEN and FILE_ATTRIBUTE_SYSTEM support (take 6)
On Do, 2009-10-01 at 16:59 -0700, Dan Kegel wrote:
+/***********************************************************************
wrap_getxattr
- Retrieve extended attribute of posix file into given buffer.
- Use fd if given, else use fname; caller only sets one or the
other.
- */
+static int wrap_getxattr(int fd, const char *fname, const char *attrname, char *attrbuf, int attrbuflen)
Code, which is called very often, should produce as less overhead as possible. I suggest to use "static inline". Seperate implementations for fd and fname should also make a difference.
Your code handle different parameter for the attr functions. Is it possible, that code, which is compiled for one ABI version can call the implementation of the other ABI version?
When Wine depends on libattr and libattr-dev after you Patch, all packagers should get this information.
On Sun, Oct 4, 2009 at 2:58 PM, Detlef Riekenberg wine.dev@web.de wrote:
Code, which is called very often, should produce as less overhead as possible. I suggest to use "static inline".
I have 'static' already. The compiler's probably better at deciding when to use inline than we are, may as well leave inline off unless we're sure it's faster. (inline can cause bloat...) I haven't done the benchmarking to test whether this change slows, say, an ls -lR down, but I suspect any slowdown will be because of the extra syscall rather than the lack of inline.
Seperate implementations for fd and fname should also make a difference.
I had that originally, but it seemed duplicative. Samba seems to have similar unified helpers, so I went with that idea.
Your code handle different parameter for the attr functions. Is it possible, that code, which is compiled for one ABI version can call the implementation of the other ABI version?
Not sure I follow. Can you rephrase that?
When Wine depends on libattr and libattr-dev after you Patch, all packagers should get this information.
It shouldn't; modern glibc handles the attr functions. The libattr checks are only for old distros. - Dan
Dan Kegel wrote:
On Sun, Oct 4, 2009 at 2:58 PM, Detlef Riekenberg wine.dev@web.de wrote:
Code, which is called very often, should produce as less overhead as possible. I suggest to use "static inline".
I have 'static' already. The compiler's probably better at deciding when to use inline than we are, may as well leave inline off unless we're sure it's faster. (inline can cause bloat...) I haven't done the benchmarking to test whether this change slows, say, an ls -lR down, but I suspect any slowdown will be because of the extra syscall rather than the lack of inline.
It doesn't really matter if you add inline or not. The C standard specifies inline as a hint and "newer" gcc versions implement it that way. Of course the hint adds to the heuristic that gcc uses to determine when to inline or not. For a discussion on "inline" see the "Who is the best inliner of all?" article on LWN http://lwn.net/Articles/314848/ .
bye michael
On Tue, Oct 6, 2009 at 10:20 PM, Dan Kegel dank@kegel.com wrote:
Your code handle different parameter for the attr functions. Is it possible, that code, which is compiled for one ABI version can call the implementation of the other ABI version?
Not sure I follow. Can you rephrase that?
I don't know if this is what Detlef means but I think the attr functions and structures differ slightly on other Unixen. At least I am pretty sure they differ enough on OS X from Linux or Solaris to cause problems. I have not looked at the recent iteration of the patch so if you already account for what I am saying, please send this message to the Null device.
Thanks
On Wed, Oct 7, 2009 at 6:59 AM, Steven Edwards winehacker@gmail.com wrote:
I don't know if this is what Detlef means but I think the attr functions and structures differ slightly on other Unixen. At least I am pretty sure they differ enough on OS X from Linux or Solaris to cause problems.
Yeah, the patch handles OS X and Linux, and is written so somebody could add Solaris support. I think Detlef was suggesting a particular way of doing that. I have a wrapper function that is implemented by each OS differently using #ifdef's. He was suggesting that instead (I think) that I use one OS's API as the wrapper function. I don't think it's a good fit in this case (though I like it in other situations). - Dan
Hi Dan,
On Oct 7, 2009, at 9:24 AM, Dan Kegel wrote:
Yeah, the patch handles OS X [...]
Tangentially related to this xattr issue, are you aware of BSD's chflags system call and the st_flags field of struct stat? On Mac OS X (and any other BSD which defines UF_HIDDEN), FILE_ATTRIBUTE_HIDDEN is better implemented by checking for UF_HIDDEN in the st_flags field. This will then actually integrate with the native API's and GUI's notion of hidden files.
Similarly, FILE_ATTRIBUTE_ARCHIVE corresponds to the _lack_ of UF_NODUMP and SF_ARCHIVED. (We should check the SF_ flag, but we can only expect to be able to set/clear the UF_ flag.)
It would also be good to check for UF_IMMUTABLE|SF_IMMUTABLE to determine FILE_ATTRIBUTE_READONLY, and set UF_IMMUTABLE when setting FILE_ATTRIBUTE_READONLY, although that's a bit of a digression from what you're directly working on in your patches.
The version of Samba for Mac OS X is patched to work with these flags:
http://www.opensource.apple.com/source/samba/samba-235/patches/support-stfla... http://www.opensource.apple.com/source/samba/samba-235/patches/support-stfla...
Regards, Ken
On Wed, Oct 7, 2009 at 9:21 AM, Ken Thomases ken@codeweavers.com wrote:
Tangentially related to this xattr issue, are you aware of BSD's chflags system call and the st_flags field of struct stat? On Mac OS X (and any other BSD which defines UF_HIDDEN), FILE_ATTRIBUTE_HIDDEN is better implemented by checking for UF_HIDDEN in the st_flags field. This will then actually integrate with the native API's and GUI's notion of hidden files.
Golly. No, wasn't aware of that, thanks.
Similarly, FILE_ATTRIBUTE_ARCHIVE corresponds to the _lack_ of UF_NODUMP and SF_ARCHIVED. (We should check the SF_ flag, but we can only expect to be able to set/clear the UF_ flag.)
I'd rather wait on that flag, it's less pressing.
It would also be good to check for UF_IMMUTABLE|SF_IMMUTABLE to determine FILE_ATTRIBUTE_READONLY, and set UF_IMMUTABLE when setting FILE_ATTRIBUTE_READONLY, although that's a bit of a digression from what you're directly working on in your patches.
Why isn't readonly enough? Geez.
The version of Samba for Mac OS X is patched to work with these flags:
http://www.opensource.apple.com/source/samba/samba-235/patches/support-stfla... http://www.opensource.apple.com/source/samba/samba-235/patches/support-stfla...
Thanks for the pointers! - Dan
On Oct 7, 2009, at 11:46 AM, Dan Kegel wrote:
On Wed, Oct 7, 2009 at 9:21 AM, Ken Thomases ken@codeweavers.com wrote:
It would also be good to check for UF_IMMUTABLE|SF_IMMUTABLE to determine FILE_ATTRIBUTE_READONLY, and set UF_IMMUTABLE when setting FILE_ATTRIBUTE_READONLY, although that's a bit of a digression from what you're directly working on in your patches.
Why isn't readonly enough? Geez.
Hmm? You mean Wine's current tactic of checking the mode flags to determine READONLY?
Well, Windows itself distinguishes the READONLY attribute as separate from having/lacking write permissions on a file.
Also, Wine's current tactic falls down in the presence of ACLs. A file may grant no permissions in its mode bits, but grant various access through ACLs. Wine considers such a file READONLY even if some users (even the current user) may write to it.
Lastly, Mac OS X presents normal permissions bits, possibly including write permission, for file systems mounted read-only. Wine reports such files as non-READONLY if they have write bits in the mode. One might consider this a Mac OS X bug, but, again, permissions are different from physical writability. ("You may, but you can't." ;-)
For those reasons, I was planning to follow your patch, when it finally gets accepted, with one to augment the current mode check with a call to access(2) if we have a file path. If access(..., W_OK) succeeds, I'll clear the READONLY attribute, regardless of what the mode bits say (but respecting the [US]F_IMMUTABLE flags). If it fails with errno == EROFS, I'll set READONLY, regardless of the mode bits.
Regards, Ken
-
Dan Kegel -
Detlef Riekenberg -
Ken Thomases -
Michael Stefaniuc -
Steven Edwards