Re: [musl] riscv32 v2

[Rich Felker <dalias@libc.org>]

On Mon, Sep 07, 2020 at 09:54:56PM -0400, Rich Felker wrote:
> On Fri, Sep 04, 2020 at 01:48:19AM -0400, Stefan O'Rear wrote:
> > From cd57a6b47783c5302f931e543b608cb3ba58387d Mon Sep 17 00:00:00 2001
> > From: Stefan O'Rear <sorear@fastmail.com>
> > Date: Thu, 3 Sep 2020 03:59:59 -0400
> > Subject: [PATCH 06/14] Only call fstatat if defined
> > 
> > riscv32 and future architectures lack it.
> > ---
> >  src/stat/fchmodat.c   | 23 ++++++++++++++++++++---
> >  src/stat/fstatat.c    |  6 ++++++
> >  src/stdio/tempnam.c   |  9 +++++++--
> >  src/stdio/tmpnam.c    |  9 +++++++--
> >  src/time/__map_file.c | 19 +++++++++++++++----
> >  5 files changed, 55 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/stat/fchmodat.c b/src/stat/fchmodat.c
> > index 4ee00b0a..857e84e5 100644
> > --- a/src/stat/fchmodat.c
> > +++ b/src/stat/fchmodat.c
> > @@ -1,8 +1,10 @@
> >  #include <sys/stat.h>
> >  #include <fcntl.h>
> >  #include <errno.h>
> > +#include <stdint.h>
> >  #include "syscall.h"
> >  #include "kstat.h"
> > +#include "statx.h"
> >  
> >  int fchmodat(int fd, const char *path, mode_t mode, int flag)
> >  {
> > @@ -11,13 +13,22 @@ 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))
> > +	mode_t get_mode = st.st_mode;
> > +#else
> > +	struct statx st;
> > +	if ((ret = __syscall(SYS_statx, fd, path, flag, STATX_TYPE, &st)))
> > +		return __syscall_ret(ret);
> > +	mode_t get_mode = st.stx_mode;
> > +#endif
> > +
> > +	if (S_ISLNK(get_mode))
> >  		return __syscall_ret(-EOPNOTSUPP);
> >  
> >  	if ((fd2 = __syscall(SYS_openat, fd, path, O_RDONLY|O_PATH|O_NOFOLLOW|O_NOCTTY|O_CLOEXEC)) < 0) {
> > @@ -27,9 +38,15 @@ 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);
> > +	get_mode = st.st_mode;
> > +#else
> > +	ret = __syscall(SYS_statx, AT_FDCWD, proc, 0, STATX_TYPE, &st);
> > +	get_mode = st.stx_mode;
> > +#endif
> >  	if (!ret) {
> > -		if (S_ISLNK(st.st_mode)) ret = -EOPNOTSUPP;
> > +		if (S_ISLNK(get_mode)) ret = -EOPNOTSUPP;
> >  		else ret = __syscall(SYS_fchmodat, AT_FDCWD, proc, mode);
> >  	}
> >  
> 
> I was just looking at this file for another reason, and wondered why
> we're not just calling the fstatat function here. There's no namespace
> reason not to. According to the description of commit
> c9ebff4736128186121424364c1c62224b02aee3, use of struct kstat here was
> done "as a low-risk fix" right before release of 1.2.0, and I probably
> had in mind changing it to fstatat later and just never got around to
> it.
> 
> The same could be done for tempnam (POSIX) but not tmpnam (C) or
> __map_file (used in plain C interfaces like setlocale and time
> functions) without adding a namespace-safe alias for fstatat.
> 
> Of course this does pull in more code that's not needed, so maybe your
> version of the change is best, and maybe this is what I had in mind
> when I hesitated to make the bigger change back in February. I don't
> like that knowledge of different syscall alternatives leaks all over
> the place, but I also don't like increasing code size unnecessarily on
> all archs...

Not something that requires action, but possible streamlining I just
discovered: in places where lstat is being used to probe file
existence (tmpnam, etc.), SYS_readlink also works, yields smaller
code, and is #ifdef-free. You get the same ENOENT for nonexistence,
and success or EINVAL indicating existence as a symlink or
non-symlink, respectively.

Rich