Messages in this thread | | | Date | Wed, 27 Sep 2023 15:06:03 +0200 | Subject | Re: [PATCH 1/1] tools/nolibc: Add workarounds for centos-7 | From | Rodrigo Campos <> |
| |
On 9/27/23 01:30, Thomas Weißschuh wrote: > thanks for your patch!
Thank you for nolibc :)
> On 2023-09-26 15:36:47+0200, Rodrigo Campos wrote: >> Centos-7 doesn't include statx on its linux/stat.h file. So, let's just >> define it if the include doesn't define STATX_BASIC_STATS. > > Could you mention which version of the kernel headers you compiled this > with and with which version you tested it?
It was on CI using vagrant, it is:
kernel-headers x86_64 3.10.0-1160.99.1.el7
And this kernel:
Linux cirrus-task-6368858685046784 3.10.0-1160.95.1.el7.x86_64 #1 SMP Mon Jul 24 13:59:37 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Luckily this info is exposed in their CI easily, see here: https://cirrus-ci.com/task/6368858685046784?logs=host_info#L1. The host_info and "Run install_dependencies" have this kind of info.
> Also which is the exact revision you use to extract nolibc?
As from Linux 6.6-rc3 (6465e260f48790807eef06b583b38ca9789b6072)
> Does nolibc actually support statx()/stat() on centos-7 with these changes?
Oh, I haven't tried that, sorry I didn't mention. We are basically just using exceve(), but the thing is that "sutrct statx" and "STATX_BASIC_STATS" are used in sys.h, so compilation fails.
And just a forward declaration of the struct won't help, as it is not just a pointer, in stat() we instantiate it.
> I'm asking because I tried to reproduce it and for me CentOS 7 with > kernel-headers 3.10.0-1160.99.1.el7 doesn't define __NR_statx. > Without this symbol the statx() and stat() functions should just always > return -ENOSYS. > It seems a bit wasteful to introduce 200 new lines of code for a "feature" > that will not do anything. > > FYI the hard requirement for the statx syscall is fairly new, it was > added in commit af93807eaef6 ("tools/nolibc: remove the old sys_stat support").
Oh, great pointer, thanks!
> As you are vendoring nolibc, if you don't need stat/statx support in > for your usecase you could drop the support for it in your vendored > copy. > Or we try to reintroduce compatibility for stat() without the statx() > syscall. But given the really limited applicability, personally I'm > against that.
We can definitely remove that struct statx bits in our vendoring. It will simplify updating if we don't have to patch it, so if we can't include a fix in nolibc, I think we will continue doing the hack ourselves and that is all. It is not too bad :)
I don't think it is worth for nolibc, at least for this use case, to reintroduce compatibility for stat() without statx().
For now we are work-arounding it by doing basically the same thing I'm doing here: https://github.com/opencontainers/runc/blob/96a61d3bf0dcc26343bfafe5112934d73d280dd3/libcontainer/dmz/xstat.h
We then include this file before nolibc.h, and then the type works as fine: https://github.com/opencontainers/runc/blob/96a61d3bf0dcc26343bfafe5112934d73d280dd3/libcontainer/dmz/_dmz.c
Would it be acceptable for nolibc if I just define what we use: * struct statx * struct statx_timestamp (used inside struct statx) * STATX_BASIC_STATS (or STATX_* constants too, as you prefer)
?
> > Some more notes below. > >> This makes nolibc work on centos-7 just fine, before this patch it >> failed with: >> >> nolibc/sys.h:987:78: warning: ‘struct statx’ declared inside parameter list [enabled by default] >> int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) >> >> Please note that while on types.h we can still include linux/stat.h >> and it won't cause any issues, it seems simpler if we just always >> include "statx.h" instead of that file and be safe. That is why I >> changed types.h too. > > All of nolibc will end up included into the same namespace by design. > It seems weird that it would make a difference from where this file is > included.
No, sorry, I just wanted to say that we should include "statx.h" instead of "<linux/stat.h>" as that does the define for old distros that don't do it.
It is true that we can include "statx.h" in nolibc.h and that should do the trick too.
>> diff --git tools/include/nolibc/statx.h tools/include/nolibc/statx.h >> new file mode 100644 >> index 000000000000..d05528754154 >> --- /dev/null >> +++ tools/include/nolibc/statx.h >> @@ -0,0 +1,218 @@ > > Below you mention that this was copied from > tools/include/uapi/linux/stat.h, but... > >> +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > > The original code was "GPL-2.0 WITH Linux-syscall-note".
Oh, great point, sorry I didn't realize. What should we do, then?
Do you know what is the proper way to just define the same types here (i.e. struct statx and what I mentioned is what we really need a few lines above)?
>> +#define S_IFMT 00170000 >> +#define S_IFSOCK 0140000 >> +#define S_IFLNK 0120000 >> +#define S_IFREG 0100000 >> +#define S_IFBLK 0060000 >> +#define S_IFDIR 0040000 >> +#define S_IFCHR 0020000 >> +#define S_IFIFO 0010000 >> +#define S_ISUID 0004000 >> +#define S_ISGID 0002000 >> +#define S_ISVTX 0001000 >> + >> +#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK) >> +#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG) >> +#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR) >> +#define S_ISCHR(m) (((m) & S_IFMT) == S_IFCHR) >> +#define S_ISBLK(m) (((m) & S_IFMT) == S_IFBLK) >> +#define S_ISFIFO(m) (((m) & S_IFMT) == S_IFIFO) >> +#define S_ISSOCK(m) (((m) & S_IFMT) == S_IFSOCK) >> + >> +#define S_IRWXU 00700 >> +#define S_IRUSR 00400 >> +#define S_IWUSR 00200 >> +#define S_IXUSR 00100 >> + >> +#define S_IRWXG 00070 >> +#define S_IRGRP 00040 >> +#define S_IWGRP 00020 >> +#define S_IXGRP 00010 >> + >> +#define S_IRWXO 00007 >> +#define S_IROTH 00004 >> +#define S_IWOTH 00002 >> +#define S_IXOTH 00001 > > We already have all of these in types.h.
Yes, I realized but I thought it was simpler conceptually if we include the whole file. This way there are no differences if the host has this or not.
I can definitely remove this if you prefer and just define what we strictly need.
>> diff --git tools/include/nolibc/sys.h tools/include/nolibc/sys.h >> index fdb6bd6c0e2f..d3e45793682a 100644 >> --- tools/include/nolibc/sys.h >> +++ tools/include/nolibc/sys.h >> @@ -20,9 +20,9 @@ >> #include <linux/time.h> >> #include <linux/auxvec.h> >> #include <linux/fcntl.h> /* for O_* and AT_* */ >> -#include <linux/stat.h> /* for statx() */ > > So this means that compatibility with user applications that also > include <linux/stat.h> on their own is broken? > That would not be good.
Hmm, no, it just means that if we want to get struct statx in all distros, including centos-7, we should use the other include that will define it for centos.
We can keep this here as long as we also include xstat.h, in some other part.
Best, Rodrigo
| |