[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200124140027.GT30412@brightrain.aerifal.cx>
Date: Fri, 24 Jan 2020 09:00:27 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] add statx
On Sun, Jan 19, 2020 at 01:12:47PM +0100, Ben Noordhuis wrote:
> glibc exposes a wrapper for this system call. it's inconvenient that
> musl doesn't so let's add one.
>
> ref: https://github.com/rust-lang/rust/pull/67774
> ---
> include/fcntl.h | 15 +++++++++++++++
> include/sys/stat.h | 34 ++++++++++++++++++++++++++++++++++
> src/stat/fstatat.c | 27 +--------------------------
> src/stat/statx.c | 8 ++++++++
> 4 files changed, 58 insertions(+), 26 deletions(-)
> create mode 100644 src/stat/statx.c
>
> diff --git a/include/fcntl.h b/include/fcntl.h
> index b664cdc4..21050a65 100644
> --- a/include/fcntl.h
> +++ b/include/fcntl.h
> @@ -106,6 +106,21 @@ int posix_fallocate(int, off_t, off_t);
> #define AT_STATX_DONT_SYNC 0x4000
> #define AT_RECURSIVE 0x8000
>
> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
latter. Is there a reason for that? Generally the extensions that are
pretty clearly Linux-only, as opposed to something that other
POSIX-based OS's are likely to adopt, are put under GNU/ALL to
discourage their use without intent and to avoid namespace clutter.
Also, I don't think it makes sense for these to be in fcntl.h at all,
and I don't think there's precedent. glibc has them exposed via
sys/stat.h not fcntl.h. And it looks like glibc exposes them (and all
the statx stuff) only under _GNU_SOURCE:
#ifdef __USE_GNU
# include <bits/statx.h>
#endif
So in summary, unless there's strong reason to do differently, move to
sys/stat.h and make _GNU_SOURCE-only.
> #define FAPPEND O_APPEND
> #define FFSYNC O_SYNC
> #define FASYNC O_ASYNC
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..5db71590 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -5,6 +5,7 @@ extern "C" {
> #endif
>
> #include <features.h>
> +#include <stdint.h>
Extra indirect headers can't be pulled in unconditionally beyond
what's specified for sys/stat.h. Instead you could put it under
_GNU_SOURCE (this is another reason not to put it in default), but
what would be better is putting just the __NEED_* for the types you
want under _GNU_SOURCE here, e.g.
#ifdef _GNU_SOURCE
#define __NEED_uint16_t
#define __NEED_uint32_t
#define __NEED_uint64_t
#define __NEED_int32_t
#define __NEED_int64_t
#endif
> int stat(const char *__restrict, struct stat *__restrict);
> int fstat(int, struct stat *);
> int lstat(const char *__restrict, struct stat *__restrict);
> @@ -93,6 +126,7 @@ int utimensat(int, const char *, const struct timespec [2], int);
>
> #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> int lchmod(const char *, mode_t);
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
Also _GNU_SOURCE-only.
> #define S_IREAD S_IRUSR
> #define S_IWRITE S_IWUSR
> #define S_IEXEC S_IXUSR
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index de165b5c..ab22e4c6 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -8,36 +8,11 @@
> #include "syscall.h"
> #include "kstat.h"
>
> -struct statx {
> - uint32_t stx_mask;
> - uint32_t stx_blksize;
> - uint64_t stx_attributes;
> - uint32_t stx_nlink;
> - uint32_t stx_uid;
> - uint32_t stx_gid;
> - uint16_t stx_mode;
> - uint16_t pad1;
> - uint64_t stx_ino;
> - uint64_t stx_size;
> - uint64_t stx_blocks;
> - uint64_t stx_attributes_mask;
> - struct {
> - int64_t tv_sec;
> - uint32_t tv_nsec;
> - int32_t pad;
> - } stx_atime, stx_btime, stx_ctime, stx_mtime;
> - uint32_t stx_rdev_major;
> - uint32_t stx_rdev_minor;
> - uint32_t stx_dev_major;
> - uint32_t stx_dev_minor;
> - uint64_t spare[14];
> -};
> -
> static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
> {
> struct statx stx;
>
> - int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
> + int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
> if (ret) return ret;
This looks ok.
> *st = (struct stat){
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..36b45d33
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,8 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include "syscall.h"
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> + return syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +}
> --
> 2.23.0
Should we do fallback translation from stat if SYS_statx fails with
ENOSYS? My leaning would be yes.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
