Messages in this thread | | | From | Feiyang Chen <> | Date | Wed, 8 Feb 2023 15:42:56 +0800 | Subject | Re: [PATCH 1/3] nolibc: Add statx() support to implement sys_stat() |
| |
On Wed, 8 Feb 2023 at 11:31, Willy Tarreau <w@1wt.eu> wrote: > > Hi Feiyang, > > Sorry for the delay. > > On Wed, Feb 08, 2023 at 10:09:48AM +0800, Feiyang Chen wrote: > > On Tue, 7 Feb 2023 at 22:31, Arnd Bergmann <arnd@arndb.de> wrote: > (...) > > > Given that all architectures implement statx the same way, I wonder > > > if we can't just kill off the old function here and always use statx. > > > > > > That would also allow removing the architecture specific > > > sys_stat_struct definitions in all arch-*.h files. > > > > > > > Hi, Arnd, > > > > I'd really like to make all architectures use sys_statx() instead > > of sys_stat(). I just fear we might get dragged into a long discussion. > > Can I send a patch series to do this later? > > 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. >
Hi, Arnd, Willy,
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?
> I'm having another comment below however: > > > > > +struct statx_timestamp { > > > > + __s64 tv_sec; > > > > + __u32 tv_nsec; > > > > + __s32 __reserved; > > > > +}; > > > > + > > > > +struct statx { > > > > + /* 0x00 */ > > > > + __u32 stx_mask; /* What results were written [uncond] */ > > > > + __u32 stx_blksize; /* Preferred general I/O size [uncond] */ > > > > + __u64 stx_attributes; /* Flags conveying information about the file > (...) > > For all these types exposed to userland that you have to define, I'd > prefer if we would avoid using kernel-inherited types like __u32, __u64 > etc given that all other archs for now only use regular types. It's not > critical at all but I'd prefer that we stay consistent between all archs. > Also, based on the comments on the fields it seems to me that this file > was just copy-pasted from some uapi header which is not under the same > license, so that's another reason for just defining what is needed here > if you need to define it here. And of course, if including linux/stat.h > also works, that's by far the preferred solution which will also save > us from having to maintain a copy! >
I try to include linux/stat.h and it works.
Thanks, Feiyang
> Thanks! > Willy
| |