Re: [musl] riscv32 v2

[Rich Felker <dalias@libc.org>]

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

Rich