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
