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

[Kamil Dudka]

On Mon October 3 2011 15:45:36 Bruno Haible wrote:
> Kamil Dudka wrote:
> > The commit 95f7c57 introduced an unintended change in behavior of ls -L.
> > I am attaching a patch that restores the old behavior.
> 
> This patch introduces an additional lstat() system call

Yes.

> , when it is not necessary.

I disagree.  ls -L does not do lstat().

> Recall that when file_has_acl is called, it gets the result of stat() or
> lstat() passed through the 'sb' argument. If it's the result of stat(),
> the user is interested in the symlink's target, so the code should call
> acl_extended_file(). If it's the result of lstat(), the user is interested
> in the symlink itself, so the code should call
> acl_extended_file_nofollow().
> 
> So there are 3 cases:
>   a) A non-symlink, hence acl_extended_file_nofollow and acl_extended_file
> are equivalent.
>   b) A symlink, with dereferencing (stat()). Must call acl_extended_file.
>   c) A symlink, without dereferencing (lstat()). Must call
>      acl_extended_file_nofollow.
> 
> You are using lstat() to distinguish case a) from b)+c). But it is also
> possible to inspect *sb, to distinguish case c) from a)+b).

How exactly?  I do not get it.

> In fact, the code already does this, at the beginning of the function
> file_has_acl. So in case c) the big compound statement is skipped, and the
> function immediately does a "return 0". The patch that you proposed on
> 2011-04-06 and committed on 2011-07-22 therefore can only have an effect in
> the cases a) and b). In the case b) you have introduced a regression,
> that's what you've discovered now. And in the case a) - when the file is
> not a symlink: are you saying in <https://bugzilla.redhat.com/720325> that
> calling acl_extended_file on a non-symlink will trigger autofs mounts??

Yes, the getxattr syscall triggers it.  lgetxattr does not.

> So, before adding more complexity to the patch of 2011-07-22, I would like
> to
>   1) understand better which file system operations triggered the autofs
>      mounts, I can't really beleive that calling getxattr on a non-symlink
>      will trigger autofs mounts.

That's the information I was given.  As a user-space hacker, I cannot help 
with providing more details about the kernel internals.

>   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.

> If you cannot come up with such a test case, I suggest to revert the patch
> of 2011-07-22.

Why?  AFAIK, there has never been a test-case for the ACL detection in ls.

Kamil