GNU bug report logs - #60620
[PATCH] copy.c: replace set_acl() with chmod_or_fchmod()

Previous Next

To reply to this bug, email your comments to 60620 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
To: bug-coreutils <at> gnu.org
Cc: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
Subject: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()
Date: Fri,  6 Jan 2023 16:23:06 +0100

This patch replaces set_acl() funclion call with chmod_or_fchmod()
Both functions works (AFAIK) the same way (at least in Linux) so should be possible.
Using chmod_or_fchmod would also help us to reduce dependency on libacl
(see the forthcoming patch to qcopy-acl.c from Gnulib).

Ondrej

---
 src/copy.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 519c43b00..1e69f3090 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -833,7 +833,7 @@ set_owner (const struct cp_options *x, char const *dst_name,
       if ((USE_ACL
            || (old_mode & CHMOD_MODE_BITS
                & (~new_mode | S_ISUID | S_ISGID | S_ISVTX)))
-          && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
+          && chmod_or_fchmod (dst_name, dest_desc, restrictive_temp_mode) != 0)
         {
           if (! owner_failure_ok (x))
             error (0, errno, _("clearing permissions for %s"),
@@ -1490,12 +1490,12 @@ copy_reg (char const *src_name, char const *dst_name,
     }
   else if (x->set_mode)
     {
-      if (set_acl (dst_name, dest_desc, x->mode) != 0)
+      if (chmod_or_fchmod (dst_name, dest_desc, x->mode) != 0)
         return_val = false;
     }
   else if (x->explicit_no_preserve_mode && *new_dst)
     {
-      if (set_acl (dst_name, dest_desc, MODE_RW_UGO & ~cached_umask ()) != 0)
+      if (chmod_or_fchmod (dst_name, dest_desc, MODE_RW_UGO & ~cached_umask ()) != 0)
         return_val = false;
     }
   else if (omitted_permissions | extra_permissions)
@@ -3060,14 +3060,14 @@ copy_internal (char const *src_name, char const *dst_name,
     }
   else if (x->set_mode)
     {
-      if (set_acl (dst_name, -1, x->mode) != 0)
+      if (chmod_or_fchmod (dst_name, -1, x->mode) != 0)
         return false;
     }
   else if (x->explicit_no_preserve_mode && new_dst)
     {
       int default_permissions = S_ISDIR (src_mode) || S_ISSOCK (src_mode)
                                 ? S_IRWXUGO : MODE_RW_UGO;
-      if (set_acl (dst_name, -1, default_permissions & ~cached_umask ()) != 0)
+      if (chmod_or_fchmod (dst_name, -1, default_permissions & ~cached_umask ()) != 0)
         return false;
     }
   else
-- 
2.39.0

Message #8 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>, 60620 <at> debbugs.gnu.org
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Sat, 7 Jan 2023 15:53:37 -0800

On 2023-01-06 07:23, Ondrej Valousek wrote:
> -          && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
> +          && chmod_or_fchmod (dst_name, dest_desc, restrictive_temp_mode) != 0)

Doesn't this sort of change require the qcopy-acl.c change in Gnulib? If 
so, we need to wait for that Gnulib change before installing this 
change, right? Otherwise we won't be copying ACLs correctly.

Message #11 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, "60620 <at> debbugs.gnu.org"
 <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Sun, 8 Jan 2023 19:20:54 +0000

[Message part 1 (text/plain, inline)]

No, these two changes are (from the functional point of view) independent - i.e. acl handling will work regardless of the order these 2 are applied.
The only difference is, that once both are applied, we could link coreutils w/o libacl


Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Paul Eggert <eggert <at> cs.ucla.edu>
Sent: Sunday, January 8, 2023 12:53:37 AM
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>; 60620 <at> debbugs.gnu.org <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()

On 2023-01-06 07:23, Ondrej Valousek wrote:
> -          && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
> +          && chmod_or_fchmod (dst_name, dest_desc, restrictive_temp_mode) != 0)

Doesn't this sort of change require the qcopy-acl.c change in Gnulib? If
so, we need to wait for that Gnulib change before installing this
change, right? Otherwise we won't be copying ACLs correctly.

[Message part 2 (text/html, inline)]

Message #14 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>, 60620 <at> debbugs.gnu.org
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Sun, 8 Jan 2023 21:04:19 +0000

On 06/01/2023 15:23, Ondrej Valousek wrote:
> This patch replaces set_acl() funclion call with chmod_or_fchmod()
> Both functions works (AFAIK) the same way (at least in Linux) so should be possible.
> Using chmod_or_fchmod would also help us to reduce dependency on libacl
> (see the forthcoming patch to qcopy-acl.c from Gnulib).

The "at least in Linux" qualification worries me.
Having a very quick look at the qset_acl() code suggests
it clears ACLs on some platforms at least,
which chmod_or_fchmod() does not.
Am I reading that wrong?

thanks,
Pádraig

Message #17 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>,
 "60620 <at> debbugs.gnu.org" <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Sun, 8 Jan 2023 15:18:59 -0800

On 2023-01-08 11:20, Ondrej Valousek wrote:
> No, these two changes are (from the functional point of view) independent - i.e. acl handling will work regardless of the order these 2 are applied.
> The only difference is, that once both are applied, we could link coreutils w/o libacl

If the change quoted below is applied, but the Gnulib change is not, 
then ACLs won't be copied. So I don't see how the two changes are 
independent.

> Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
> ________________________________
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Sent: Sunday, January 8, 2023 12:53:37 AM
> To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>; 60620 <at> debbugs.gnu.org <60620 <at> debbugs.gnu.org>
> Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()
> 
> On 2023-01-06 07:23, Ondrej Valousek wrote:
>> -          && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
>> +          && chmod_or_fchmod (dst_name, dest_desc, restrictive_temp_mode) != 0)
> 
> Doesn't this sort of change require the qcopy-acl.c change in Gnulib? If
> so, we need to wait for that Gnulib change before installing this
> change, right? Otherwise we won't be copying ACLs correctly.
> 

Message #20 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, "60620 <at> debbugs.gnu.org"
 <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Mon, 9 Jan 2023 06:38:11 +0000

[Message part 1 (text/plain, inline)]

Not sure if I understand what you talk about.
Qset_acl() is not copying any ACLs.
How it could affect the code change we do to the qcopy_acl()? There is no change there...


Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Paul Eggert <eggert <at> cs.ucla.edu>
Sent: Monday, January 9, 2023 12:18:59 AM
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>; 60620 <at> debbugs.gnu.org <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()

On 2023-01-08 11:20, Ondrej Valousek wrote:
> No, these two changes are (from the functional point of view) independent - i.e. acl handling will work regardless of the order these 2 are applied.
> The only difference is, that once both are applied, we could link coreutils w/o libacl

If the change quoted below is applied, but the Gnulib change is not,
then ACLs won't be copied. So I don't see how the two changes are
independent.

> Zasláno z Outlooku pro Android<https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Faka.ms%2FAAb9ysg&data=05%7C01%7Condrej.valousek.xm%40renesas.com%7C170dde217c084a2a39e208daf1cebaff%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638088167448100493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U865sX5YrDSgZhGguA4b0p5i7oqeQ1OS%2BiTbqJMNiew%3D&reserved=0>
> ________________________________
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Sent: Sunday, January 8, 2023 12:53:37 AM
> To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>; 60620 <at> debbugs.gnu.org <60620 <at> debbugs.gnu.org>
> Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()
>
> On 2023-01-06 07:23, Ondrej Valousek wrote:
>> -          && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
>> +          && chmod_or_fchmod (dst_name, dest_desc, restrictive_temp_mode) != 0)
>
> Doesn't this sort of change require the qcopy-acl.c change in Gnulib? If
> so, we need to wait for that Gnulib change before installing this
> change, right? Otherwise we won't be copying ACLs correctly.
>

[Message part 2 (text/html, inline)]

Message #23 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
To: Pádraig Brady <P <at> draigBrady.com>,
 "60620 <at> debbugs.gnu.org" <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Mon, 9 Jan 2023 06:53:04 +0000

[Message part 1 (text/plain, inline)]

Well, true, I do not know for sure what will happen on different platforms, yes - I can't test.
On Linux both work the same way.
Either case, the purpose of qset_acl() is to set ACLs the way so that it corresponds with the mode argument right?
That's what I would expect how it's supposed to work.
If yes, then isn't it better to handle this task to the filesystem driver in kernel and just call chmod?

Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Pádraig Brady <pixelbeat <at> gmail.com> on behalf of Pádraig Brady <P <at> draigBrady.com>
Sent: Sunday, January 8, 2023 10:04:19 PM
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>; 60620 <at> debbugs.gnu.org <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with chmod_or_fchmod()

On 06/01/2023 15:23, Ondrej Valousek wrote:
> This patch replaces set_acl() funclion call with chmod_or_fchmod()
> Both functions works (AFAIK) the same way (at least in Linux) so should be possible.
> Using chmod_or_fchmod would also help us to reduce dependency on libacl
> (see the forthcoming patch to qcopy-acl.c from Gnulib).

The "at least in Linux" qualification worries me.
Having a very quick look at the qset_acl() code suggests
it clears ACLs on some platforms at least,
which chmod_or_fchmod() does not.
Am I reading that wrong?

thanks,
Pádraig

[Message part 2 (text/html, inline)]

Message #26 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>,
 "60620 <at> debbugs.gnu.org" <60620 <at> debbugs.gnu.org>
Subject: Re: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Sun, 8 Jan 2023 23:48:12 -0800

On 2023-01-08 22:38, Ondrej Valousek wrote:
> Not sure if I understand what you talk about.
> Qset_acl() is not copying any ACLs.
> How it could affect the code change we do to the qcopy_acl()? There is no change there...

Well, perhaps I'm just misunderstanding the code.

Message #29 received at 60620 <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Valousek <ondrej.valousek.xm <at> renesas.com>
To: Pádraig Brady <P <at> draigBrady.com>,
 "60620 <at> debbugs.gnu.org" <60620 <at> debbugs.gnu.org>
Subject: RE: bug#60620: [PATCH] copy.c: replace set_acl() with
 chmod_or_fchmod()
Date: Tue, 24 Jan 2023 08:14:37 +0000

> The "at least in Linux" qualification worries me.
> Having a very quick look at the qset_acl() code suggests it clears ACLs on some platforms at least, which chmod_or_fchmod() does not.
> Am I reading that wrong?

You are right.
The question is - why do we need to clear ACLs?
The only real example the code in question used is via "cp --no-preserve=mode" where I can't think of a reason why we intentionally clear ACLs

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.