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

[Kamil Dudka]

On Mon October 3 2011 15:02:20 Bruno Haible wrote:
> The function name should explain the semantics of the function. The fact
> that it's a wrapper around acl_extended_file is something one can see by
> reading the code.
> 
> Maybe call it acl_extended_file_optimized?

Sounds good.

> > +    /* acl_extended_file_nofollow() uses lgetxattr() in order to prevent
> > +       unnecessary mounts, but it returns the same result as we already
> > +       know that NAME is not a symbolic link at this point (modulo the
> > +       TOCTTOU race condition).  */
> 
> I have a hard time understanding this comment.
> - Why the "but"? The "same result" as what?

Fixed.  I am not a native speaker.

> - What is the "TOCTTOU race condition"?

TOC = checking whether the file is a symlink
TOU = checking whether the file has ACLs

> If it's important, please add a FIXME and an explanation.

No, it does not feel important to me, at least not for ls.

> How about this comment, right after the inner #if: 1. Describe the problem.
> 2. Describe the solution.
> 
>   /* acl_extended_file() tests whether a file has an ACL.  But it can
> trigger unnecessary autofs mounts.  In newer versions of libacl, a
> function acl_extended_file_nofollow() is available that uses lgetxattr()
> and therefore does not have this problem.  It is equivalent to
>      acl_extended_file(), except on symbolic links.  */

Comment replaced.  Thanks for the suggestion.

Kamil

0001-file-has-acl-revert-unintended-change-in-behavior-of.patch
Description: Text Data