From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 14430 invoked from network); 3 Sep 2020 16:05:19 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 3 Sep 2020 16:05:19 -0000 Received: (qmail 17960 invoked by uid 550); 3 Sep 2020 16:05:15 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17942 invoked from network); 3 Sep 2020 16:05:14 -0000 Date: Thu, 3 Sep 2020 12:05:02 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200903160502.GD3265@brightrain.aerifal.cx> References: <20200903112309.102601-1-sorear@fastmail.com> <20200903112309.102601-7-sorear@fastmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200903112309.102601-7-sorear@fastmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 06/14] Only call fstatat if defined On Thu, Sep 03, 2020 at 07:23:01AM -0400, Stefan O'Rear wrote: > riscv32 and future architectures lack it. > --- > src/stat/fchmodat.c | 22 +++++++++++++++++++++- > src/stat/fstatat.c | 6 ++++++ > src/stdio/tempnam.c | 7 +++++++ > src/stdio/tmpnam.c | 7 +++++++ > src/time/__map_file.c | 13 ++++++++++++- > 5 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/src/stat/fchmodat.c b/src/stat/fchmodat.c > index 4ee00b0a..bcd44cc8 100644 > --- a/src/stat/fchmodat.c > +++ b/src/stat/fchmodat.c > @@ -1,8 +1,10 @@ > #include > #include > #include > +#include > #include "syscall.h" > #include "kstat.h" > +#include "statx.h" > > int fchmodat(int fd, const char *path, mode_t mode, int flag) > { > @@ -11,14 +13,24 @@ int fchmodat(int fd, const char *path, mode_t mode, int flag) > if (flag != AT_SYMLINK_NOFOLLOW) > return __syscall_ret(-EINVAL); > > - struct kstat st; > int ret, fd2; > char proc[15+3*sizeof(int)]; > > +#ifdef SYS_fstatat > + struct kstat st; > if ((ret = __syscall(SYS_fstatat, fd, path, &st, flag))) > return __syscall_ret(ret); > + > if (S_ISLNK(st.st_mode)) > return __syscall_ret(-EOPNOTSUPP); > +#else > + struct statx st; > + if ((ret = __syscall(SYS_statx, fd, path, flag, STATX_TYPE, &st))) > + return __syscall_ret(ret); > + > + if (S_ISLNK(st.stx_mode)) > + return __syscall_ret(-EOPNOTSUPP); > +#endif Can you do this as mode_t mode = st.st[x]_mode; inside the conditional block and unconditional if (S_ISLNK(mode)) ... outside? This ensures that the actual logic is not duplicated so it can't become inconsistent. > @@ -27,11 +39,19 @@ int fchmodat(int fd, const char *path, mode_t mode, int flag) > } > > __procfdname(proc, fd2); > +#ifdef SYS_fstatat > ret = __syscall(SYS_fstatat, AT_FDCWD, proc, &st, 0); > if (!ret) { > if (S_ISLNK(st.st_mode)) ret = -EOPNOTSUPP; > else ret = __syscall(SYS_fchmodat, AT_FDCWD, proc, mode); > } > +#else > + ret = __syscall(SYS_statx, AT_FDCWD, proc, 0, STATX_TYPE, &st); > + if (!ret) { > + if (S_ISLNK(st.stx_mode)) ret = -EOPNOTSUPP; > + else ret = __syscall(SYS_fchmodat, AT_FDCWD, proc, mode); > + } > +#endif Same here where even more logic is duplicated. > diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c > index 230a83fc..0486f21a 100644 > --- a/src/stat/fstatat.c > +++ b/src/stat/fstatat.c > @@ -45,6 +45,7 @@ static int fstatat_statx(int fd, const char *restrict path, struct stat *restric > return 0; > } > > +#ifdef SYS_fstatat > static int fstatat_kstat(int fd, const char *restrict path, struct stat *restrict st, int flag) > { > int ret; > @@ -106,15 +107,20 @@ static int fstatat_kstat(int fd, const char *restrict path, struct stat *restric > > return 0; > } > +#endif > > int fstatat(int fd, const char *restrict path, struct stat *restrict st, int flag) > { > int ret; > +#ifdef SYS_fstatat > if (sizeof((struct kstat){0}.st_atime_sec) < sizeof(time_t)) { > ret = fstatat_statx(fd, path, st, flag); > if (ret!=-ENOSYS) return __syscall_ret(ret); > } > ret = fstatat_kstat(fd, path, st, flag); > +#else > + ret = fstatat_statx(fd, path, st, flag); > +#endif > return __syscall_ret(ret); > } I'd like to have this streamlined more without duplicating the call, but it's not really any big deal, just a little bit ugly, and I don't see a better way to do it while preserving use of SYS_fstatat everywhere it's sufficient. > diff --git a/src/stdio/tempnam.c b/src/stdio/tempnam.c > index 565df6b6..023ff422 100644 > --- a/src/stdio/tempnam.c > +++ b/src/stdio/tempnam.c > @@ -5,8 +5,10 @@ > #include > #include > #include > +#include > #include "syscall.h" > #include "kstat.h" > +#include "statx.h" > > #define MAXTRIES 100 > > @@ -40,8 +42,13 @@ char *tempnam(const char *dir, const char *pfx) > #ifdef SYS_lstat > r = __syscall(SYS_lstat, s, &(struct kstat){0}); > #else > +#ifdef SYS_fstatat > r = __syscall(SYS_fstatat, AT_FDCWD, s, > &(struct kstat){0}, AT_SYMLINK_NOFOLLOW); > +#else > + r = __syscall(SYS_statx, AT_FDCWD, s, AT_SYMLINK_NOFOLLOW, 0, > + &(struct statx){0}); > +#endif > #endif Can be #elif. Also, is zero request mask ok here? > diff --git a/src/stdio/tmpnam.c b/src/stdio/tmpnam.c > index d667a836..c63dabcb 100644 > --- a/src/stdio/tmpnam.c > +++ b/src/stdio/tmpnam.c > @@ -4,8 +4,10 @@ > #include > #include > #include > +#include > #include "syscall.h" > #include "kstat.h" > +#include "statx.h" > > #define MAXTRIES 100 > > @@ -20,8 +22,13 @@ char *tmpnam(char *buf) > #ifdef SYS_lstat > r = __syscall(SYS_lstat, s, &(struct kstat){0}); > #else > +#ifdef SYS_fstatat > r = __syscall(SYS_fstatat, AT_FDCWD, s, > &(struct kstat){0}, AT_SYMLINK_NOFOLLOW); > +#else > + r = __syscall(SYS_statx, AT_FDCWD, s, AT_SYMLINK_NOFOLLOW, 0, > + &(struct statx){0}); > +#endif > #endif Same. > if (r == -ENOENT) return strcpy(buf ? buf : internal, s); > } > diff --git a/src/time/__map_file.c b/src/time/__map_file.c > index d3cefa82..f80752bc 100644 > --- a/src/time/__map_file.c > +++ b/src/time/__map_file.c > @@ -1,19 +1,30 @@ > +#define _BSD_SOURCE > #include > #include > #include > +#include > #include "syscall.h" > #include "kstat.h" > +#include "statx.h" > > const char unsigned *__map_file(const char *pathname, size_t *size) > { > - struct kstat st; > const unsigned char *map = MAP_FAILED; > int fd = sys_open(pathname, O_RDONLY|O_CLOEXEC|O_NONBLOCK); > if (fd < 0) return 0; > +#ifdef SYS_fstat > + struct kstat st; > if (!syscall(SYS_fstat, fd, &st)) { > map = __mmap(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0); > *size = st.st_size; > } > +#else > + struct statx st; > + if (!syscall(SYS_statx, fd, "", AT_EMPTY_PATH, STATX_SIZE, &st)) { > + map = __mmap(0, st.stx_size, PROT_READ, MAP_SHARED, fd, 0); > + *size = st.stx_size; > + } > +#endif As in fchmodat, etc., save the st[x]_size field in the #ifdef block and get the common code out so it's not duplicated. Rich