Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L

[Kamil Dudka]

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