Re: Review request for glibc system call wrapper for statx

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 05/07/2018 14:13, Florian Weimer wrote:
> I've proposed a statx system call wrapper for glibc:
> 
>   https://sourceware.org/ml/libc-alpha/2018-06/msg01038.html
> 
> The somewhat questionable part is the userspace emulation if the kernel does not support statx.  It looks like this:
> 
> +/* Approximate emulation of statx.  This will always fill in
> +   POSIX-mandated attributes even if the underlying file system does
> +   not actually support it (for example, GID and UID on file systems
> +   without UNIX-style permissions).  */
> +static __attribute__ ((unused)) int
> +statx_generic (int fd, const char *path, int flags,
> +               unsigned int mask, struct statx *buf)
> +{
> +  /* Flags which need to be cleared before passing them to
> +     fstatat64.  */
> +  static const int clear_flags = AT_STATX_SYNC_AS_STAT;
> +
> +    /* Flags supported by our emulation.  */
> +  static const int supported_flags
> +    = AT_EMPTY_PATH | AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW
> +      | clear_flags;
> +
> +  if (__glibc_unlikely ((flags & ~supported_flags) != 0))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +
> +  struct stat64 st;
> +  int ret = __fstatat64 (fd, path, &st, flags & ~clear_flags);
> +  if (ret != 0)
> +    return ret;
> +
> +  *buf = (struct statx)
> +    {
> +      /* We copy everything from fstat64, which corresponds the basic
> +         fstat64.  */
> +      .stx_mask = STATX_BASIC_STATS,
> +      .stx_blksize = st.st_blksize,
> +      .stx_nlink = st.st_nlink,
> +      .stx_uid = st.st_uid,
> +      .stx_gid = st.st_gid,
> +      .stx_mode = st.st_mode,
> +      .stx_ino = st.st_ino,
> +      .stx_size = st.st_size,
> +      .stx_blocks = st.st_blocks,
> +      .stx_atime = statx_convert_timestamp (st.st_atim),
> +      .stx_ctime = statx_convert_timestamp (st.st_ctim),
> +      .stx_mtime = statx_convert_timestamp (st.st_mtim),
> +      .stx_rdev_major = __gnu_dev_major (st.st_rdev),
> +      .stx_rdev_minor = __gnu_dev_minor (st.st_rdev),
> +      .stx_dev_major = __gnu_dev_minor (st.st_dev),
> +      .stx_dev_minor = __gnu_dev_minor (st.st_dev),
> +    };
> +
> +  return 0;
> +}
> 
> Do you think this emulation is a good idea?  Or should we drop it and just return ENOSYS?

I think it should a reasonable expectation that if kernel does not 
support statx, it behaves as fstatat since the function semantic
have the stx_mask exactly to advertise its internal support (which
ideally would allow kernel to setup the underlying filesystem support,
but I think it also should work for userspace emulation). 

And since it would take some time to get the minimum kernel to one
required for statx (4.11), I think not failing on the fallback should
ok as well.

(as a side note afaik there is no gain in using static for flags
definition).