[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220827181018.GA11483@brightrain.aerifal.cx>
Date: Sat, 27 Aug 2022 14:10:19 -0400
From: Rich Felker <dalias@...c.org>
To: Duncan Bellamy <dunk@...kimushi.com>
Cc: info@...ordhuis.nl, musl@...ts.openwall.com
Subject: Re: [PATCH 1/1] resubmitting old statx patch with changes
On Sat, Aug 27, 2022 at 03:57:52PM +0100, Duncan Bellamy wrote:
> ---
> include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> src/stat/fstatat.c | 27 +------------------------
> src/stat/statx.c | 35 +++++++++++++++++++++++++++++++++
> 3 files changed, 85 insertions(+), 26 deletions(-)
> create mode 100644 src/stat/statx.c
>
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..81424462 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -5,6 +5,7 @@ extern "C" {
> #endif
>
> #include <features.h>
> +#include <stdint.h>
This can't be done unconditionally. Either the #include directive
needs to be within the preprocessor conditional where statx is, or the
individual __NEED_uint32_t etc need to be defined conditional on the
same condition before bits/alltypes.h is included. The latter is
probably better here.
> #define __NEED_dev_t
> #define __NEED_ino_t
> @@ -70,6 +71,54 @@ extern "C" {
> #define UTIME_NOW 0x3fffffff
> #define UTIME_OMIT 0x3ffffffe
>
> +#if defined(_GNU_SOURCE)
> +#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
> +
> +struct statx_timestamp {
> + int64_t tv_sec;
> + uint32_t tv_nsec;
> + int32_t __pad;
> +};
Minor nit but this could probably just be tv_nsec, __pad (both same
type). This also eliminates the gratuitous need to expose the signed
32-bit type which is not used elsewhere.
I was looking at whether *all* of the types here could be replaced
with equivalent ones that don't require exposing extra types, but the
ones documented as uint64_t probably can't. All the uint32_t in
principle could just be unsigned, and tv_sec time_t, but...
> +
> +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 __pad0[1];
> + uint64_t stx_ino;
> + uint64_t stx_size;
> + uint64_t stx_blocks;
> + uint64_t stx_attributes_mask;
> + struct statx_timestamp stx_atime;
> + struct statx_timestamp stx_btime;
> + struct statx_timestamp stx_ctime;
> + struct statx_timestamp stx_mtime;
> + uint32_t stx_rdev_major;
> + uint32_t stx_rdev_minor;
> + uint32_t stx_dev_major;
> + uint32_t stx_dev_minor;
> + uint64_t __pad1[14];
> +};
stx_ino etc. should not be assuming ino_t etc. are defined the same as
uint64_t rather than something like unsigned long long. So we probably
just go with writing the types as documented...
> +
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> +#endif
> int stat(const char *__restrict, struct stat *__restrict);
> int fstat(int, struct stat *);
> int lstat(const char *__restrict, struct stat *__restrict);
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index 74c51cf5..5b2248a9 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -7,36 +7,11 @@
> #include <sys/sysmacros.h>
> #include "syscall.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;
>
> *st = (struct stat){
This can be a separate change from adding statx, but it's easy to
separate when merging.
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..ff49841b
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,35 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include <syscall.h>
> +#include <sys/sysmacros.h>
> +#include <errno.h>
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> + int ret = syscall(SYS_statx, dirfd, path, flags, mask, stx);
> + if (ret == ENOSYS) {
> + struct stat st;
> + fstatat(dir_fd, path, &st, flags);
> + stx.stx_dev_major = major(st.st_dev);
> + stx.stx_dev_minor = minor(st.st_dev);
> + stx.stx_ino = st.st_ino;
> + stx.stx_mode = st.st_mode;
> + stx.stx_nlink = st.st_nlink;
> + stx.stx_uid = st.st_uid;
> + stx.stx_gid = st.st_gid;
> + stx.stx_size = st.st_size;
> + stx.stx_blksize = st.st_blksize;
> + stx.stx_blocks = st.st_blocks;
> + stx.stx_atime.tv_sec = st.st_atim.tv_sec;
> + stx.stx_atime.tv_nsec = st.st_atim.tv_nsec;
> + stx.stx_mtime.tv_sec = st.st_mtim.tv_sec;
> + stx.stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> + stx.stx_ctime.tv_sec = st.st_ctim.tv_sec;
> + stx.stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> + stx.stx_btime = 0;
> + stx.stx_mask = STATX_BASIC_STATS;
> + ret = EINVAL;
> + }
> +
> + return ret;
> +}
> --
> 2.34.1
I think this wasn't tested because it won't even compile (stx. instead
of stx->). It's also wrongly assuming syscall() returned a positive
error code rather than -1 on error and setting errno, but you should
be using the __syscall form that returns a negated error code, then
__syscall_ret at the end. I would write it as:
int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
if (ret != -ENOSYS) return __syscall_ret(ret);
then the fallback case outside a conditional. The fallback can't
assume fstatat succeeded like you're doing either. It needs to return
-1 immediately if fstatat fails.
All of this code needs to be conditional on SYS_fstatat existing, as
new archs don't have it and only have SYS_statx.
One annoying thing about this but I don't know a good fix; maybe you
have an idea: if SYS_statx fails with ENOSYS, the call to fstatat will
immediately perform SYS_statx again, only to have it fail, then
finally fall back to SYS_fstatat or other syscalls after two failures.
I'm not sure if it makes sense to expose __fstatat_kstat
libc-internally (hidden) to use here or do something else; that's
kinda getting into more complexity around this than I'd like for the
sake of optimizing old systems.
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.
