mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Dunk <dunk@denkimushi.com>
To: musl@lists.openwall.com
Cc: info@bnoordhuis.nl
Subject: Re: [musl] [PATCH 1/1] resubmitting old statx patch with changes
Date: Sun, 28 Aug 2022 00:11:30 +0100	[thread overview]
Message-ID: <96BC8712-13B0-41C6-852B-ABF791B9A20A@denkimushi.com> (raw)
In-Reply-To: <20220827181018.GA11483@brightrain.aerifal.cx>


> On 27 Aug 2022, at 19:10, Rich Felker <dalias@libc.org> wrote:
> 
> 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.

I didn’t understand the later so did the former.

>> #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.

changed

> 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.

moved to separate commit

>> 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.

Yes it wasn’t compiled, I copied the old patch and code from `fstatat_statx` for copying the data.  Compiled it locally now.

> 
> All of this code needs to be conditional on SYS_fstatat existing, as
> new archs don't have it and only have SYS_statx.

Changed to this

> 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.

I don’t really understand the fallback mechanism, or why statx would fail and fstatat work on some systems.  Maybe use defines at compile time so either one is used or the other? 

> Rich

Duncan

  reply	other threads:[~2022-08-27 23:11 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-19 12:12 [musl] [PATCH] add statx Ben Noordhuis
2020-01-24  8:38 ` [musl] " Ben Noordhuis
2020-01-24 14:01   ` Rich Felker
2020-01-28  8:59     ` Ben Noordhuis
2020-01-28 13:39       ` Rich Felker
2020-01-24 14:00 ` [musl] " Rich Felker
2020-01-24 15:27   ` Florian Weimer
2020-01-24 15:54     ` Rich Felker
2020-01-24 16:12       ` Florian Weimer
2020-01-24 16:29         ` Rich Felker
2020-01-28 10:41           ` Florian Weimer
2020-01-28 13:18             ` Rich Felker
2020-02-17  9:10               ` Florian Weimer
2020-02-17 15:29                 ` Rich Felker
2022-08-27 14:57 ` [musl] [PATCH 0/1] " Duncan Bellamy
2022-08-27 14:57   ` [musl] [PATCH 1/1] resubmitting old statx patch with changes Duncan Bellamy
2022-08-27 18:10     ` Rich Felker
2022-08-27 23:11       ` Dunk [this message]
2022-08-27 23:11 ` [musl] [PATCH 0/2] V2 Duncan Bellamy
2022-08-27 23:11   ` [musl] [PATCH 1/2] V2 resubmitting old statx patch with changes Duncan Bellamy
2022-08-29 13:50     ` [musl] " Dunk
2022-08-27 23:11   ` [musl] [PATCH 2/2] V2 src/stat/fstatat.c use new statx define Duncan Bellamy
2022-08-31 19:07 ` [musl] [PATCH 0/2] V3 Duncan Bellamy
2022-08-31 19:07   ` [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes Duncan Bellamy
2024-02-24 16:56     ` Rich Felker
2022-08-31 19:07   ` [musl] [PATCH 2/2] V3 src/stat/fstatat.c use new statx define Duncan Bellamy
2024-02-24 16:57     ` Rich Felker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=96BC8712-13B0-41C6-852B-ABF791B9A20A@denkimushi.com \
    --to=dunk@denkimushi.com \
    --cc=info@bnoordhuis.nl \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).