Re: [PATCH 1/3] nolibc: Add statx() support to implement sys_stat()

[Feiyang Chen]

On Wed, 8 Feb 2023 at 11:31, Willy Tarreau <w@xxxxxx> 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@xxxxxxxx> 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