From: Paul Eggert <eggert@cs.ucla.edu>
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: bug-gnulib@gnu.org, kdudka@redhat.com
Subject: Re: [PATCH] Use xattr (Linux) in copy-acl.c
Date: Tue, 3 Jan 2023 12:40:58 -0800 [thread overview]
Message-ID: <f3b7375a-ef32-d5c2-c1d3-779ecb591716@cs.ucla.edu> (raw)
In-Reply-To: <20230103140816.1121699-1-ondrej.valousek.xm@renesas.com>
Thanks again for looking into this.
On 2023-01-03 06:08, Ondrej Valousek wrote:
> +static int
> +copy_attr_permissions (const char *name, struct error_context *ctx)
> +{
> + int action = attr_copy_action (name, ctx);
> + return action == ATTR_ACTION_PERMISSIONS;
> +}
This returns bool not int, so it should be declared to return bool. The
body can be a one-liner, no?
> +#ifdef USE_XATTR
> + ret = chmod_or_fchmod (dst_name, dest_desc, mode);
> + /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs
> + * (Posix or NFSv4). Of course, that won't address ACLs conversion
> + * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
> + */
> + if(ret == 0)
> + ret = source_desc <= 0 && dest_desc <= 0
> + ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
> + : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);
Please add brief commentary as to why we need both chmod_or_fchmod and
attr_copy_* (i.e., why it doesn't suffice to just do the attr_copy_*),
and why it's important to do chmod_or_fchmod before doing attr_copy_*?
Also, please use Gnulib style in comments; no "*" on each line.
Also, the test "source_desc <= 0 && dest_desc <= 0" looks wrong.
Shouldn't it be "source_desc < 0 || dest_desc < 0"?
> + if test $ac_cv_header_attr_libattr_h = yes \
> + && test $ac_cv_header_attr_error_context_h = yes; then
Although quoting inside "" isn't needed for shell variables that you
know have only safe characters, these ac_cv_* shell var uses should be
quoted inside "" for safety, e.g., 'test "$ac_cv_header_attr_libattr_h"
= yes', in case the user screws up and puts spaces into their values.
Please do this systematically for ac_cv_* variables.
next prev parent reply other threads:[~2023-01-03 20:41 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-03 14:08 [PATCH] Use xattr (Linux) in copy-acl.c Ondrej Valousek
2023-01-03 16:44 ` Bruno Haible
2023-01-03 20:40 ` Paul Eggert [this message]
2023-01-03 20:53 ` Bruno Haible
-- strict thread matches above, loose matches on Subject: below --
2023-01-04 10:03 Ondrej Valousek
2023-01-04 11:27 ` Bruno Haible
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.gnu.org/mailman/listinfo/bug-gnulib
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f3b7375a-ef32-d5c2-c1d3-779ecb591716@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=bug-gnulib@gnu.org \
--cc=kdudka@redhat.com \
--cc=ondrej.valousek.xm@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).