Messages in this thread | | | From | Feiyang Chen <> | Date | Wed, 8 Feb 2023 17:20:18 +0800 | Subject | Re: [PATCH 1/3] nolibc: Add statx() support to implement sys_stat() |
| |
On Wed, 8 Feb 2023 at 16:19, Willy Tarreau <w@1wt.eu> wrote: > > On Wed, Feb 08, 2023 at 09:06:24AM +0100, Arnd Bergmann wrote: > > On Wed, Feb 8, 2023, at 08:42, Feiyang Chen wrote: > > > On Wed, 8 Feb 2023 at 11:31, Willy Tarreau <w@1wt.eu> wrote: > > >> > > >> I generally agree with the Arnd's points overall and I'm fine with the > > >> rest of your series. On this specific point, I'm fine with your proposal, > > >> let's just start with sys_statx() only on this arch, please add a comment > > >> about this possibility in the commit message that brings statx(), > > >> indicating that other archs are likely to benefit from it as well, and > > >> let's see after this if we can migrate all archs to statx. > > >> > > > > > > We have a problem if we just start with sys_statx() only on this arch. > > > When struct stat is not defined, what should we do with stat() in the > > > nolibc selftest? > > > > To clarify: your proposed implementation of the stat() function that > > fills the nolibc 'struct stat' based on information from 'struct statx' > > is fine here. Just remove the 'struct sys_stat_struct' definition > > loongarch but keep 'struct stat'. > > Ah I think now I understand the problem Feiyang is facing. Since "struct > stat" is just between libc and userland, there's the "sys_stat_struct" > that we're using to abstract the syscalls in sys_stat() and that is > compatible with each variant of the stat() syscall on each arch. Here > there's simply no stat() syscall so it's best not to try to abstract > this function at all since types will not match between stat and statx, > so it will be better to just implement it like this: > > #if defined(__NR_statx) > static __attribute__((unused)) > int sys_stat(const char *path, struct stat *buf) > { > struct statx statx; > long ret; > > ret = statx(...); > buf->xxx = statx.xxx; > ... > return ret; > } > #else ... > // keep the regular sys_stat() here > #endif > > Also looking at the man page I see that statx() only appeared in 4.11, > and here we're targetting userland so I'd rather keep a bit of margin > when it comes to backwards compatibility, thus not dropping stat() and > friends too early when not necessary. However using statx() by default > when available sounds fine to me! >
Hi, Arnd, Willy,
I think I get it now, thank you!
Thanks, Feiyang
> Cheers, > Willy
| |