bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Bruno Haible
Subject: Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L
Date: Mon, 3 Oct 2011 15:02:20 +0200
User-agent: KMail/1.13.6 (Linux/2.6.37.6-0.5-desktop; KDE/4.6.0; x86_64; ; )

Kamil Dudka wrote:
> Thanks in advance for considering the patch!

> +#if (HAVE_ACL_EXTENDED_FILE) /* Linux */

Unnecessary parentheses.

> +acl_extended_file_wrap (char const *name)

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?

> +# if HAVE_ACL_EXTENDED_FILE_NOFOLLOW
> +#endif /* HAVE_ACL_EXTENDED_FILE_NOFOLLOW */

Incorrect indentation of the #endif line.

> +    /* 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?
- What is the "TOCTTOU race condition"? If it's important, please add a FIXME
  and an explanation.

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

Bruno



reply via email to

[Prev in Thread] Current Thread [Next in Thread]