Re: [COREUTILS 2/2] ls: Don't treat lack of acl support as an error

[Andreas Grünbacher]

2015-04-21 5:40 GMT+02:00 Pádraig Brady <address@hidden>:
> 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.