| Message ID | ZTu/JJVcvHBScVeN@waldemar-brodkorb.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [uclibc-ng-devel] Re: [PATCH 1/4] fstatat64: define it as a wrapper of statx if the kernel does not support fstatat64 syscall | expand |
Hi Waldemar, It's just not clear to me what __UCLIBC_HAVE_STATX__ is supposed to mean. Does it mean the kernel supports the syscall? Or does it mean the arch want to expose some "statx" function which wraps the statx syscall? (because statx is non POSIX and is Linux specific) In the first scenario I would think that it seems redundant with just checking __NR_statx In the second scenario I would think that even if the arch does not want to expose a statx() function in the libc, the arch would still want to wrap the "old" functions (fstatat, stat, etc) around the new statx syscall, right? In any case I am interested in understanding which of the 2 scenario is correct: what does __UCLIBC_HAVE_STATX__ mean? Also, indeed I can see that in other files the check is done like this `#if defined __NR_statx && defined __UCLIBC_HAVE_STATX__` but maybe we need to make sure this is correct, and if it's not, fix this. I hope I'm helping and not making this more complex than it's already is :) Cheers, Yann Le 27/10/2023 à 15:46, Waldemar Brodkorb a écrit : > Hi Yann, > > I added __UCLIBC_HAVE_STATX__ for c-sky when I first added statx > support. What you think about attached patch to fix the mips64 n32 > issue? It showed no further regressions while testing. > > best regards > Waldemar > > Yann Sionneau wrote, > >> Hi Waldemar, >> >> This is due to statx structure being defined in statx.h >> >> statx.h being included in sys/stat.h conditionally to __UCLIBC_HAVE_STATX__ >> being defined. Which is not, for mips arch. >> >> defining __UCLIBC_HAVE_STATX__ in >> libc/sysdeps/linux/mips/bits/uClibc_arch_features.h does the job and >> everything compiles OK. >> >> BUT, I'm not sure if this is the best way of fixing things... >> >> I am wondering why we use this __UCLIBC_HAVE_STATX__ macro in the libc >> instead of testing the existence of the syscall in the kernel like for every >> other syscall by doing `#if defined(__NR_statx)` ? >> >> Or maybe __UCLIBC_HAVE_STATX__ means something else? Maybe it means we want >> to expose a real statx() function wrapper to userspace? >> >> If this is the case, that would mean fstatat.c would need to include >> directly bits/statx.h instead of sys/stat.h BUT it's explicitly forbidden: >> >> ``` >> >> #ifndef _SYS_STAT_H >> # error Never include <bits/statx.h> directly, include <sys/stat.h> instead. >> #endif >> >> ``` >> >> So... I'm a bit hesitant here on what's the correct thing to do. >> >> Any thoughts on the list? >> >> Regards, >> >> Yann >> >> Le 21/10/2023 à 07:50, Waldemar Brodkorb a écrit : >>> Hi Yann, >>> >>> it seems I haven't tested this series good enough. >>> It breaks mips64n32 build with following error: >>> >>> libc/sysdeps/linux/common/fstatat.c: In function 'fstatat': >>> libc/sysdeps/linux/common/fstatat.c:41:22: error: storage size of 'tmp' isn't known >>> 41 | struct statx tmp; >>> | ^~~ >>> In file included from ./include/sys/syscall.h:33, >>> from libc/sysdeps/linux/common/fstatat.c:9: >>> ./include/bits/syscalls.h:290:48: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] >>> 290 | register ARG_TYPE __a1 __asm__("$5") = (ARG_TYPE) arg2; \ >>> | ^ >>> ./include/bits/syscalls.h:49:9: note: in expansion of macro 'internal_syscall5' >>> 49 | internal_syscall##nr (, "li\t$2, %2\t\t\t# " #name "\n\t", \ >>> | ^~~~~~~~~~~~~~~~ >>> ./include/bits/syscalls.h:24:24: note: in expansion of macro 'INTERNAL_SYSCALL' >>> 24 | long result_var = INTERNAL_SYSCALL(name, err, nr, args); \ >>> | ^~~~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:43:15: note: in expansion of macro 'INLINE_SYSCALL' >>> 43 | ret = INLINE_SYSCALL(statx, 5, fd, file, flag, >>> | ^~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:44:30: error: 'STATX_BASIC_STATS' undeclared (first use in this function) >>> 44 | STATX_BASIC_STATS, &tmp); >>> | ^~~~~~~~~~~~~~~~~ >>> ./include/bits/syscalls.h:292:59: note: in definition of macro 'internal_syscall5' >>> 292 | register ARG_TYPE __a3 __asm__("$7") = (ARG_TYPE) arg4; \ >>> | ^~~~ >>> ./include/bits/syscalls.h:24:24: note: in expansion of macro 'INTERNAL_SYSCALL' >>> 24 | long result_var = INTERNAL_SYSCALL(name, err, nr, args); \ >>> | ^~~~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:43:15: note: in expansion of macro 'INLINE_SYSCALL' >>> 43 | ret = INLINE_SYSCALL(statx, 5, fd, file, flag, >>> | ^~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:44:30: note: each undeclared identifier is reported only once for each function it appears in >>> 44 | STATX_BASIC_STATS, &tmp); >>> | ^~~~~~~~~~~~~~~~~ >>> ./include/bits/syscalls.h:292:59: note: in definition of macro 'internal_syscall5' >>> 292 | register ARG_TYPE __a3 __asm__("$7") = (ARG_TYPE) arg4; \ >>> | ^~~~ >>> ./include/bits/syscalls.h:24:24: note: in expansion of macro 'INTERNAL_SYSCALL' >>> 24 | long result_var = INTERNAL_SYSCALL(name, err, nr, args); \ >>> | ^~~~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:43:15: note: in expansion of macro 'INLINE_SYSCALL' >>> 43 | ret = INLINE_SYSCALL(statx, 5, fd, file, flag, >>> | ^~~~~~~~~~~~~~ >>> libc/sysdeps/linux/common/fstatat.c:41:22: warning: unused variable 'tmp' [-Wunused-variable] >>> 41 | struct statx tmp; >>> | ^~~ >>> gmake[6]: *** [Makerules:369: libc/sysdeps/linux/common/fstatat.os] Error 1 >>> >>> Any idea? >>> >>> It is with uClibc-ng current git. >>> >>> Thanks in advance >>> Waldemar >>> >>> >>> yann@sionneau.net wrote, >>> >>>> From: Yann Sionneau<ysionneau@kalray.eu> >>>> >>>> Define fstatat64 as a wrapper of statx if the kernel does not support fstatat64 syscall >>>> This is the case for non-legacy architectures that don't define __ARCH_WANT_NEW_STAT >>>> in their linux arch/xxx/include/asm/unistd.h >>>> >>>> Signed-off-by: Yann Sionneau<ysionneau@kalray.eu> >>>> >>>> --- >>>> libc/sysdeps/linux/common/fstatat64.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/libc/sysdeps/linux/common/fstatat64.c b/libc/sysdeps/linux/common/fstatat64.c >>>> index 711521a6a..836ed4114 100644 >>>> --- a/libc/sysdeps/linux/common/fstatat64.c >>>> +++ b/libc/sysdeps/linux/common/fstatat64.c >>>> @@ -43,5 +43,28 @@ int fstatat64(int fd, const char *file, struct stat64 *buf, int flag) >>>> } >>>> libc_hidden_def(fstatat64) >>>> #else >>>> + >>>> +#if defined(__NR_statx) && defined(__UCLIBC_HAVE_STATX__) >>>> +# include <sys/stat.h> >>>> +# include <statx_cp.h> >>>> +# include <fcntl.h> // for AT_NO_AUTOMOUNT >>>> + >>>> +int fstatat64(int fd, const char *file, struct stat64 *buf, int flag) >>>> +{ >>>> + struct statx tmp; >>>> + >>>> + int r = INLINE_SYSCALL(statx, 5, fd, file, AT_NO_AUTOMOUNT | flag, >>>> + STATX_BASIC_STATS, &tmp); >>>> + >>>> + if (r != 0) >>>> + return r; >>>> + >>>> + __cp_stat_statx ((struct stat *)buf, &tmp); >>>> + >>>> + return 0; >>>> +} >>>> +libc_hidden_def(fstatat64) >>>> +#endif >>>> + >>>> /* should add emulation with fstat64() and /proc/self/fd/ ... */ >>>> #endif >>>> -- >>>> 2.42.0 >>>> >>>> _______________________________________________ >>>> devel mailing list --devel@uclibc-ng.org >>>> To unsubscribe send an email todevel-leave@uclibc-ng.org >>>>
Hi Yann, Yann Sionneau wrote, > Hi Waldemar, > > It's just not clear to me what __UCLIBC_HAVE_STATX__ is supposed to mean. > > Does it mean the kernel supports the syscall? I would say yes. It is for newer architectures which only support statx like c-sky. At least that is the reason it was introduced. The old functions are not available for c-sky. Now it is also used by kvx. > Or does it mean the arch want to expose some "statx" function which wraps the > statx syscall? (because statx is non POSIX and is Linux specific) > > In the first scenario I would think that it seems redundant with just checking > __NR_statx It seems not to be redundant, as you can see it breaks some architectures which in newer kernels define __NR_statx, but the old functions still are available and working fine like mips64 n32. > In the second scenario I would think that even if the arch does not want to > expose a statx() function in the libc, the arch would still want to wrap the > "old" functions (fstatat, stat, etc) around the new statx syscall, right? > > In any case I am interested in understanding which of the 2 scenario is > correct: what does __UCLIBC_HAVE_STATX__ mean? > > Also, indeed I can see that in other files the check is done like this `#if > defined __NR_statx && defined __UCLIBC_HAVE_STATX__` but maybe we need to make > sure this is correct, and if it's not, fix this. I at least hope this is correct, yes. > I hope I'm helping and not making this more complex than it's already is :) Is it now sufficiently explained? Can I push the change? best regards Waldemar
Hi Waldemar, Le 27/10/2023 à 18:02, Waldemar Brodkorb a écrit : > Hi Yann, > Yann Sionneau wrote, > >> Hi Waldemar, >> >> It's just not clear to me what __UCLIBC_HAVE_STATX__ is supposed to mean. >> >> Does it mean the kernel supports the syscall? > I would say yes. It is for newer architectures which only support > statx like c-sky. At least that is the reason it was introduced. > The old functions are not available for c-sky. > Now it is also used by kvx. > >> Or does it mean the arch want to expose some "statx" function which wraps the >> statx syscall? (because statx is non POSIX and is Linux specific) >> >> In the first scenario I would think that it seems redundant with just checking >> __NR_statx > It seems not to be redundant, as you can see it breaks some > architectures which in newer kernels define __NR_statx, but the old > functions still are available and working fine like mips64 n32. > >> In the second scenario I would think that even if the arch does not want to >> expose a statx() function in the libc, the arch would still want to wrap the >> "old" functions (fstatat, stat, etc) around the new statx syscall, right? >> >> In any case I am interested in understanding which of the 2 scenario is >> correct: what does __UCLIBC_HAVE_STATX__ mean? >> >> Also, indeed I can see that in other files the check is done like this `#if >> defined __NR_statx && defined __UCLIBC_HAVE_STATX__` but maybe we need to make >> sure this is correct, and if it's not, fix this. > I at least hope this is correct, yes. Ok > >> I hope I'm helping and not making this more complex than it's already is :) > Is it now sufficiently explained? Can I push the change? I wanted to spend some time to think about this more, but I must confess that I don't have the time right now ... I'm a bit swamped. I don't want to delay the fix any longer so let's push it if you think it's the correct fix :) Sorry for delaying! Regards,
From 223d46f66af17e18be6e1eb2933a2c70eb64ad47 Mon Sep 17 00:00:00 2001 From: Waldemar Brodkorb <wbx@openadk.org> Date: Fri, 27 Oct 2023 15:42:34 +0200 Subject: [PATCH] depend on __UCLIBC_HAVE_STATX__ Fixes compilation issues on mips64 n32. Signed-off-by: Waldemar Brodkorb <wbx@openadk.org> --- libc/sysdeps/linux/common/fstatat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libc/sysdeps/linux/common/fstatat.c b/libc/sysdeps/linux/common/fstatat.c index db4a8327f..13611e9d0 100644 --- a/libc/sysdeps/linux/common/fstatat.c +++ b/libc/sysdeps/linux/common/fstatat.c @@ -32,7 +32,7 @@ int fstatat(int fd, const char *file, struct stat *buf, int flag) libc_hidden_def(fstatat) #else -#if defined(__NR_statx) +#if defined(__NR_statx) && defined __UCLIBC_HAVE_STATX__ #include <sys/sysmacros.h> // for makedev int fstatat(int fd, const char *file, struct stat *buf, int flag) -- 2.30.2