Re: "ls -l": Avoid unnecessary getxattr() overhead

[Jim Meyering]

Pádraig Brady wrote:

> On 02/09/2012 02:17 PM, Jim Meyering wrote:
>> Pádraig Brady wrote:
>>> On 02/09/2012 11:32 AM, Jim Meyering wrote:
>>>> Technically, we could probably exempt all files on that device, but
>>>> ...
>>>
>>> So you avoid symlinks as they can point outside the device.
>>> Unfortunately so can bind mounts, so you probably have to key on device?
>>
>> Thanks.  That confirms my impression that my patch was not worthwhile:
>> It was buggy, too.
>>
>> I suppose we could use a little hash table, whose entries are
>> <dev_t device_number, bool getfilecon_required> pairs.
>>
>> Or even a single static dev_t selinux_challenged_device,
>> that if equal to f->stat.st_dev, we can skip the *getfilecon call.
>> That should be good enough for most uses.
>>
>> Here's that simpler patch:
>
> That's nice and clean and looks correct.

Thanks for the speedy review.
Now I'll have to write comments, log, and tests.

>> diff --git a/src/ls.c b/src/ls.c
>> index f5cd37a..cb9f834 100644
>> --- a/src/ls.c
>> +++ b/src/ls.c
>> @@ -2777,6 +2777,45 @@ clear_files (void)
>>    file_size_width = 0;
>>  }
>>
>> +static bool
>> +errno_unsupported (int err)
>> +{
>> +  return err == ENOTSUP || err == ENODATA || err == EOPNOTSUPP;
>> +}
>
> Hmm, could one get ENODATA for only some files on a file system?

Yes, I suspect so.

>>From getfilecon(2) "If the context does not exist, or the process
> has no access to this attribute, errno is set to ENODATA.".

Good catch.
I had changed that to remove the ENODATA term on one system (and in
the comment you see below) but somehow used the old version of that
function above.  That'll teach me not to copy diffs around, rather than
real change-sets.

>>  /* Add a file to the current table of files.
>>     Verify that the file exists, and print an error message if it does not.
>>     Return the number of blocks that the file occupies.  */
>> @@ -2919,8 +2958,8 @@ gobble_file (char const *name, enum filetype type, 
>> ino_t inode,
>>            bool have_selinux = false;
>>            bool have_acl = false;
>>            int attr_len = (do_deref
>> -                          ?  getfilecon (absolute_name, &f->scontext)
>> -                          : lgetfilecon (absolute_name, &f->scontext));
>> +                          ? getfilecon_cache (absolute_name, f)
>> +                          : lgetfilecon_cache (absolute_name, f));
>>            err = (attr_len < 0);
>>
>>            if (err == 0)
>>
>
>
> The same logic could be applied to file_has_acl()
> and cap_get_file(), which both return errno=ENOTSUP
> when I tested against /proc here.

Yep, at least for file_has_acl which I looked at,
though at least in the few file systems where I tried it,
I always got ENODATA, which is what prompted me to remove ENODATA
from one definition of errno_unsupported.

I'm not as enthusiastic about adding code to optimize away
the calls in file_has_acl, unless there are mainstream system/FS
for which it makes a difference.