GNU bug report logs - #20310
[COREUTILS 0/2] Improved acl handling

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20310 in the body.
You can then email your comments to 20310 AT debbugs.gnu.org in the normal way.

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

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Andreas Gruenbacher <andreas.gruenbacher <at> gmail.com>, 
 bug-gnulib <at> gnu.org, bug-coreutils <at> gnu.org
Subject: Re: [COREUTILS 0/2] Improved acl handling
Date: Sun, 12 Apr 2015 16:07:53 +0100

On 12/04/15 15:37, Andreas Gruenbacher wrote:
> Hello,
> 
> here are the simple changes for pulling in the gnulib acl handling improvements
> into coreutils.
> 
> 
> These patches (and the richacl patches on top) are available here:
> 
>   https://github.com/andreas-gruenbacher/coreutils

Cool thanks. All this looks fine from a quick scan.
I'll test some more later.

BTW you could have forked from https://github.com/coreutils/

For my reference, I see the meat of the richacl support at:
https://github.com/andreas-gruenbacher/gnulib/commit/637394ca4

thanks,
Pádraig.

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Andreas Gruenbacher <andreas.gruenbacher <at> gmail.com>, 
 bug-gnulib <at> gnu.org, 20310 <at> debbugs.gnu.org
Subject: Re: [COREUTILS 2/2] ls: Don't treat lack of acl support as an error
Date: Tue, 21 Apr 2015 04:40:00 +0100

On 12/04/15 15:37, Andreas Gruenbacher wrote:
> * src/ls.c (file_has_acl_cache): When a file system doesn't support
> acls, fail with errno set to ENOTSUP.
> (gobble_file): Don't treat lack of acl support as an error.
> ---
>  src/ls.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ls.c b/src/ls.c
> index b308dd3..884e042 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -2866,7 +2866,7 @@ getfilecon_cache (char const *file, struct fileinfo *f, bool deref)
>  
>  /* Cache file_has_acl failure, when it's trivial to do.
>     Like file_has_acl, but when F's st_dev says it's on a file
> -   system lacking ACL support, return 0 with ENOTSUP immediately.  */
> +   system lacking ACL support, fail with ENOTSUP immediately.  */
>  static int
>  file_has_acl_cache (char const *file, struct fileinfo *f)
>  {
> @@ -2877,14 +2877,11 @@ file_has_acl_cache (char const *file, struct fileinfo *f)
>    if (f->stat.st_dev == unsupported_device)
>      {
>        errno = ENOTSUP;
> -      return 0;
> +      return -1;
>      }
>  
> -  /* Zero errno so that we can distinguish between two 0-returning cases:
> -     "has-ACL-support, but only a default ACL" and "no ACL support". */
> -  errno = 0;
>    int n = file_has_acl (file, &f->stat);
> -  if (n <= 0 && errno_unsupported (errno))
> +  if (n < 0 && errno_unsupported (errno))
>      unsupported_device = f->stat.st_dev;
>    return n;
>  }
> @@ -3076,7 +3073,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
>            if (err == 0 && format == long_format)
>              {
>                int n = file_has_acl_cache (absolute_name, f);
> -              err = (n < 0);
> +              err = (n < 0 && ! errno_unsupported (errno));
>                have_acl = (0 < n);
>              }

I dislike this change actually.
Or more accurately, the gnulib change that changed the file_has_acl()
interface, requiring this change.

Previously in gnulib we mapped ENOTSUP to return 0 using:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/acl-errno-valid.c

Since we've now changed file_has_acl() to return -1 in this case,
all gnulib users may now be printing erroneous errors etc.

Is there any reason not to use the same gnulib acl_errno_valid() logic
in the newly added "avoiding libacl" path?

thanks,
Pádraig.

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

From: Andreas Grünbacher <andreas.gruenbacher <at> gmail.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 20310 <at> debbugs.gnu.org, bug-gnulib <at> gnu.org
Subject: Re: [COREUTILS 2/2] ls: Don't treat lack of acl support as an error
Date: Tue, 28 Apr 2015 01:10:19 +0200

2015-04-21 5:40 GMT+02:00 Pádraig Brady <P <at> draigbrady.com>:
> On 12/04/15 15:37, Andreas Gruenbacher wrote:
>> * src/ls.c (file_has_acl_cache): When a file system doesn't support
>> acls, fail with errno set to ENOTSUP.
>> (gobble_file): Don't treat lack of acl support as an error.
>> ---
>>  src/ls.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/ls.c b/src/ls.c
>> index b308dd3..884e042 100644
>> --- a/src/ls.c
>> +++ b/src/ls.c
>> @@ -2866,7 +2866,7 @@ getfilecon_cache (char const *file, struct fileinfo *f, bool deref)
>>
>>  /* Cache file_has_acl failure, when it's trivial to do.
>>     Like file_has_acl, but when F's st_dev says it's on a file
>> -   system lacking ACL support, return 0 with ENOTSUP immediately.  */
>> +   system lacking ACL support, fail with ENOTSUP immediately.  */
>>  static int
>>  file_has_acl_cache (char const *file, struct fileinfo *f)
>>  {
>> @@ -2877,14 +2877,11 @@ file_has_acl_cache (char const *file, struct fileinfo *f)
>>    if (f->stat.st_dev == unsupported_device)
>>      {
>>        errno = ENOTSUP;
>> -      return 0;
>> +      return -1;
>>      }
>>
>> -  /* Zero errno so that we can distinguish between two 0-returning cases:
>> -     "has-ACL-support, but only a default ACL" and "no ACL support". */
>> -  errno = 0;
>>    int n = file_has_acl (file, &f->stat);
>> -  if (n <= 0 && errno_unsupported (errno))
>> +  if (n < 0 && errno_unsupported (errno))
>>      unsupported_device = f->stat.st_dev;
>>    return n;
>>  }
>> @@ -3076,7 +3073,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
>>            if (err == 0 && format == long_format)
>>              {
>>                int n = file_has_acl_cache (absolute_name, f);
>> -              err = (n < 0);
>> +              err = (n < 0 && ! errno_unsupported (errno));
>>                have_acl = (0 < n);
>>              }
>
> I dislike this change actually.
> Or more accurately, the gnulib change that changed the file_has_acl()
> interface, requiring this change.
>
> Previously in gnulib we mapped ENOTSUP to return 0 using:
> http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/acl-errno-valid.c
>
> Since we've now changed file_has_acl() to return -1 in this case,
> all gnulib users may now be printing erroneous errors etc.
>
> Is there any reason not to use the same gnulib acl_errno_valid() logic
> in the newly added "avoiding libacl" path?
>
> thanks,
> Pádraig.

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

From: Andreas Grünbacher <andreas.gruenbacher <at> gmail.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 20310 <at> debbugs.gnu.org, bug-gnulib <at> gnu.org
Subject: Re: [COREUTILS 2/2] ls: Don't treat lack of acl support as an error
Date: Tue, 28 Apr 2015 13:48:13 +0200

2015-04-21 5:40 GMT+02:00 Pádraig Brady <P <at> draigbrady.com>:
> I dislike this change actually.
> Or more accurately, the gnulib change that changed the file_has_acl()
> interface, requiring this change.
>
> Previously in gnulib we mapped ENOTSUP to return 0 using:
> http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/acl-errno-valid.c
>
> Since we've now changed file_has_acl() to return -1 in this case,
> all gnulib users may now be printing erroneous errors etc.
>
> Is there any reason not to use the same gnulib acl_errno_valid() logic
> in the newly added "avoiding libacl" path?

Indeed, it was not a good idea to change file_has_acl() in a way that
will cause other users to silently fail. I dislike the calling convention
used, it's just bizarre, but let's leave it the way it is right now.

I have updated my repositories on github:

  https://github.com/andreas-gruenbacher/gnulib
  https://github.com/andreas-gruenbacher/coreutils

Any progress with the qset_acl and qcopy_acl rewrite on non-Linux platforms?

Thanks for your work and your review.

Andreas

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

From: Andreas Grünbacher <andreas.gruenbacher <at> gmail.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 20310 <at> debbugs.gnu.org, bug-gnulib <at> gnu.org
Subject: Re: [COREUTILS 2/2] ls: Don't treat lack of acl support as an error
Date: Tue, 28 Apr 2015 22:09:17 +0200

2015-04-28 13:48 GMT+02:00 Andreas Grünbacher <andreas.gruenbacher <at> gmail.com>:
> Any progress with the qset_acl and qcopy_acl rewrite on non-Linux platforms?

(I've just pushed minor fixes to my github repositories for that.)

Thanks,
Andreas

Previous Next

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