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

[Pádraig Brady]

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.

> 
> 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?
>From getfilecon(2) "If the context does not exist, or the process
has no access to this attribute, errno is set to ENODATA.".

> +
> +/* st_dev of the most recently processed device for which
> +   we've found that getfilecon or lgetfilecon fails with
> +   e.g., ENOTSUP or EOPNOTSUPP.  */
> +static dev_t selinux_challenged_device;
> +
> +static int
> +getfilecon_cache (char const *file, struct fileinfo *f)
> +{
> +  if (f->stat.st_dev == selinux_challenged_device)
> +    {
> +      errno = ENOTSUP;
> +      return -1;
> +    }
> +  int r = getfilecon (file, &f->scontext);
> +  if (r < 0 && errno_unsupported (errno))
> +    selinux_challenged_device = f->stat.st_dev;
> +  return r;
> +}
> +
> +static int
> +lgetfilecon_cache (char const *file, struct fileinfo *f)
> +{
> +  if (f->stat.st_dev == selinux_challenged_device)
> +    {
> +      errno = ENOTSUP;
> +      return -1;
> +    }
> +  int r = lgetfilecon (file, &f->scontext);
> +  if (r < 0 && errno_unsupported (errno))
> +    selinux_challenged_device = f->stat.st_dev;
> +  return r;
> +}
> +
>  /* 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.

cheers,
Pádraig.