On Monday 03 October 2011 18:25:10 Bruno Haible wrote: > Kamil Dudka wrote: > > > 2) see a test case added to gnulib or coreutils. > > > > A while ago, Jim wrote a test-case for coreutils that catches exactly the > > bug that the original patch introduced. > > I'm asking for a test case also for the bug that the original patch fixed.
Fair enough. I am adding this to my TODO list, although there are more urgent issues preventing me from working on this atm. > Thanks for that background. So we're looking at "ls -l /media" or > "ls -ld mountpoint". Since you've mentioned symlinks, we also have to > consider symlinks to mountpoints. Well, symlinks to mount points were not mentioned in that bug. I would need to ask kernel guys whether symlinks to mount points are worth to consider in this fix at all. > So in fact we have 9 cases: > a) A non-symlink, non-directory. Here acl_extended_file_nofollow and > acl_extended_file are equivalent. If I understand this, you expect non-directories cannot be mount points, thus the call cannot trigger the mount, right? > b) A directory that is not a mount point. > c) A mount point. > d) A symlink to a file, with dereferencing (stat()). > e) A symlink to a directory that is not a mount point, with dereferencing > (stat()). f) A symlink to a mount point, with dereferencing (stat()). > g) A symlink to a file, without dereferencing (lstat()). > h) A symlink to a directory that is not a mount point, without > dereferencing (lstat()). i) A symlink to a mount point, without > dereferencing (lstat()). > > Remember the option '-L' of 'ls' is meant to mean dereference a symbolic > link, but does not mean a different treatment of mount points. But in the > kernel, the difference between getxattr and lgetxattr is a flags word of > LOOKUP_FOLLOW vs. 0 and - apparently, like you say - makes a difference for > mount points. The behavior of ls wrt. tregerring mounts is implementation defined, isn't it? Let's take it such and just choose the implementaion that makes sense. > So how do these 9 cases need to be handled: > a) Either acl_extended_file_nofollow or acl_extended_file will do. > b) Either acl_extended_file_nofollow or acl_extended_file will do. > c) Must call acl_extended_file_nofollow. > d) Either use acl_extended_file, or find the target filename through > canonicalize_filename_mode(CAN_EXISTING), then > acl_extended_file_nofollow. e) Either detect the non-mount-point through > statvfs() or similar then call acl_extended_file, or must find the target > filename through > canonicalize_filename_mode(CAN_EXISTING) then call > acl_extended_file_nofollow. f) Must find the target filename through > canonicalize_filename_mode(CAN_EXISTING), then > acl_extended_file_nofollow. g) Must return 0. > h) Must return 0. > i) Must return 0. > > The original code mishandled the cases c), e), f). > The patch from 2011-07-22 corrected case c) and mishandled the cases d), > e), f). Your current patch corrects the cases d), e), but still mishandles > case f). And it adds an extra, unnecessary system call in case a). The Problem is that case a) is not detectable on its own. > I'm trying to avoid useless system calls. If > ! S_ISLNK (sb->st_mode) && ! S_ISDIR (sb->st_mode) > then we are in case a) or d), and then no additional lstat() call is > needed. Since case a) is the most frequent one, I would find it worth to do > this check for S_ISDIR (sb->st_mode) since it can save an lstat() call. > > So I'm asking to change > > ret = acl_extended_file_wrap (name); > > to > > if (S_ISDIR (sb->st_mode)) > ret = acl_extended_file_wrap (name); > else > ret = acl_extended_file (name); Is optimization the only purpose of this change? I would then prefer to get it working first and optimize it later on. > But still still does not fix the problem of case f). Here, with your > current patch, "ls -lLd symlink-to-mountpoint" will call > acl_extended_file(), which will call getxattr(), which will activate an > autofs mount. Case f) has never worked, nobody complained. Partial fix is better than no fix. The last patch does not break anything that worked before, does it? > IMO, there are two solutions to this: > - Either convince the kernel people to introduce a different flag, > keeping LOOKUP_FOLLOW for symlinks only, This is non-starter. Kernel developers are not interested in optimizing out stat() calls from user-space. There is a similar, more serious, bug open for years. Nobody cares. https://bugzilla.redhat.com/501848 > - Or use code equivalent to canonicalize_filename_mode(CAN_EXISTING). I am personally not intersted in changing something that works unless there is a user that needs the change actually. Kamil
