I was trying to use nolibc in runc[1] and I found that it doesn't work on
centos-7.
I don't know if you care about centos-7 support, let me know. But the fix is so
simple, that I'm just sending it.
Best,
Rodrigo
[1]: https://github.com/opencontainers/runc/pull/4024/
Rodrigo Campos (1):
tools/nolibc: Add workarounds for centos-7
tools/include/nolibc/statx.h | 218 +++++++++++++++++++++++++++++++++++
tools/include/nolibc/sys.h | 2 +-
tools/include/nolibc/types.h | 2 +-
3 files changed, 220 insertions(+), 2 deletions(-)
create mode 100644 tools/include/nolibc/statx.h
--
2.40.1
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.
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.
Signed-off-by: Rodrigo Campos <[email protected]>
---
tools/include/nolibc/statx.h | 218 +++++++++++++++++++++++++++++++++++
tools/include/nolibc/sys.h | 2 +-
tools/include/nolibc/types.h | 2 +-
3 files changed, 220 insertions(+), 2 deletions(-)
create mode 100644 tools/include/nolibc/statx.h
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 @@
+/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
+/*
+ * Compatibility header to allow using statx() on old distros.
+ * Copyright (C) 2023 Rodrigo Campos Catelin <[email protected]>
+ */
+
+#ifndef _NOLIBC_STATX_H
+#define _NOLIBC_STATX_H
+
+/* We should always include this file instead of linux/stat.h, so nolibc works
+ * in old distros too.
+ *
+ * The problem is centos-7, that doesn't have statx() defined in linux/stat.h.
+ * We can't include sys/stat.h because it creates conflicts, so let's just
+ * define it here.
+ * No other distros seem affected by this, so we can remove this file when it
+ * hits EOL (06/2024).
+ */
+#include <linux/stat.h> /* for statx() */
+
+#ifndef STATX_BASIC_STATS
+
+/* This is just a c&p from tools/include/uapi/linux/stat.h as it is in
+ * Linux 6.6-rc3.
+ * We don't need it all, but it's easier to just copy it all in case in the
+ * future we start using more of it, as we won't have CI running on centos-7.
+ */
+#include <linux/types.h>
+
+#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
+
+#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
+
+#endif
+
+/*
+ * Timestamp structure for the timestamps in struct statx.
+ *
+ * tv_sec holds the number of seconds before (negative) or after (positive)
+ * 00:00:00 1st January 1970 UTC.
+ *
+ * tv_nsec holds a number of nanoseconds (0..999,999,999) after the tv_sec time.
+ *
+ * __reserved is held in case we need a yet finer resolution.
+ */
+struct statx_timestamp {
+ __s64 tv_sec;
+ __u32 tv_nsec;
+ __s32 __reserved;
+};
+
+/*
+ * Structures for the extended file attribute retrieval system call
+ * (statx()).
+ *
+ * The caller passes a mask of what they're specifically interested in as a
+ * parameter to statx(). What statx() actually got will be indicated in
+ * st_mask upon return.
+ *
+ * For each bit in the mask argument:
+ *
+ * - if the datum is not supported:
+ *
+ * - the bit will be cleared, and
+ *
+ * - the datum will be set to an appropriate fabricated value if one is
+ * available (eg. CIFS can take a default uid and gid), otherwise
+ *
+ * - the field will be cleared;
+ *
+ * - otherwise, if explicitly requested:
+ *
+ * - the datum will be synchronised to the server if AT_STATX_FORCE_SYNC is
+ * set or if the datum is considered out of date, and
+ *
+ * - the field will be filled in and the bit will be set;
+ *
+ * - otherwise, if not requested, but available in approximate form without any
+ * effort, it will be filled in anyway, and the bit will be set upon return
+ * (it might not be up to date, however, and no attempt will be made to
+ * synchronise the internal state first);
+ *
+ * - otherwise the field and the bit will be cleared before returning.
+ *
+ * Items in STATX_BASIC_STATS may be marked unavailable on return, but they
+ * will have values installed for compatibility purposes so that stat() and
+ * co. can be emulated in userspace.
+ */
+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 [uncond] */
+ /* 0x10 */
+ __u32 stx_nlink; /* Number of hard links */
+ __u32 stx_uid; /* User ID of owner */
+ __u32 stx_gid; /* Group ID of owner */
+ __u16 stx_mode; /* File mode */
+ __u16 __spare0[1];
+ /* 0x20 */
+ __u64 stx_ino; /* Inode number */
+ __u64 stx_size; /* File size */
+ __u64 stx_blocks; /* Number of 512-byte blocks allocated */
+ __u64 stx_attributes_mask; /* Mask to show what's supported in stx_attributes */
+ /* 0x40 */
+ struct statx_timestamp stx_atime; /* Last access time */
+ struct statx_timestamp stx_btime; /* File creation time */
+ struct statx_timestamp stx_ctime; /* Last attribute change time */
+ struct statx_timestamp stx_mtime; /* Last data modification time */
+ /* 0x80 */
+ __u32 stx_rdev_major; /* Device ID of special file [if bdev/cdev] */
+ __u32 stx_rdev_minor;
+ __u32 stx_dev_major; /* ID of device containing file [uncond] */
+ __u32 stx_dev_minor;
+ /* 0x90 */
+ __u64 stx_mnt_id;
+ __u32 stx_dio_mem_align; /* Memory buffer alignment for direct I/O */
+ __u32 stx_dio_offset_align; /* File offset alignment for direct I/O */
+ /* 0xa0 */
+ __u64 __spare3[12]; /* Spare space for future expansion */
+ /* 0x100 */
+};
+
+/*
+ * Flags to be stx_mask
+ *
+ * Query request/result mask for statx() and struct statx::stx_mask.
+ *
+ * These bits should be set in the mask argument of statx() to request
+ * particular items when calling statx().
+ */
+#define STATX_TYPE 0x00000001U /* Want/got stx_mode & S_IFMT */
+#define STATX_MODE 0x00000002U /* Want/got stx_mode & ~S_IFMT */
+#define STATX_NLINK 0x00000004U /* Want/got stx_nlink */
+#define STATX_UID 0x00000008U /* Want/got stx_uid */
+#define STATX_GID 0x00000010U /* Want/got stx_gid */
+#define STATX_ATIME 0x00000020U /* Want/got stx_atime */
+#define STATX_MTIME 0x00000040U /* Want/got stx_mtime */
+#define STATX_CTIME 0x00000080U /* Want/got stx_ctime */
+#define STATX_INO 0x00000100U /* Want/got stx_ino */
+#define STATX_SIZE 0x00000200U /* Want/got stx_size */
+#define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
+#define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
+#define STATX_BTIME 0x00000800U /* Want/got stx_btime */
+#define STATX_MNT_ID 0x00001000U /* Got stx_mnt_id */
+#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
+
+#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
+
+#ifndef __KERNEL__
+/*
+ * This is deprecated, and shall remain the same value in the future. To avoid
+ * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
+ * instead.
+ */
+#define STATX_ALL 0x00000fffU
+#endif
+
+/*
+ * Attributes to be found in stx_attributes and masked in stx_attributes_mask.
+ *
+ * These give information about the features or the state of a file that might
+ * be of use to ordinary userspace programs such as GUIs or ls rather than
+ * specialised tools.
+ *
+ * Note that the flags marked [I] correspond to the FS_IOC_SETFLAGS flags
+ * semantically. Where possible, the numerical value is picked to correspond
+ * also. Note that the DAX attribute indicates that the file is in the CPU
+ * direct access state. It does not correspond to the per-inode flag that
+ * some filesystems support.
+ *
+ */
+#define STATX_ATTR_COMPRESSED 0x00000004 /* [I] File is compressed by the fs */
+#define STATX_ATTR_IMMUTABLE 0x00000010 /* [I] File is marked immutable */
+#define STATX_ATTR_APPEND 0x00000020 /* [I] File is append-only */
+#define STATX_ATTR_NODUMP 0x00000040 /* [I] File is not to be dumped */
+#define STATX_ATTR_ENCRYPTED 0x00000800 /* [I] File requires key to decrypt in fs */
+#define STATX_ATTR_AUTOMOUNT 0x00001000 /* Dir: Automount trigger */
+#define STATX_ATTR_MOUNT_ROOT 0x00002000 /* Root of a mount */
+#define STATX_ATTR_VERITY 0x00100000 /* [I] Verity protected file */
+#define STATX_ATTR_DAX 0x00200000 /* File is currently in DAX state */
+
+#endif // STATX_BASIC_STATS
+
+#endif // _NOLIBC_STATX_H
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() */
#include <linux/prctl.h>
+#include "statx.h" /* for statx() */
#include "arch.h"
#include "errno.h"
#include "types.h"
diff --git tools/include/nolibc/types.h tools/include/nolibc/types.h
index 8cfc4c860fa4..ccef87e9044c 100644
--- tools/include/nolibc/types.h
+++ tools/include/nolibc/types.h
@@ -10,9 +10,9 @@
#include "std.h"
#include <linux/mman.h>
#include <linux/reboot.h> /* for LINUX_REBOOT_* */
-#include <linux/stat.h>
#include <linux/time.h>
+#include "statx.h"
/* Only the generic macros and types may be defined here. The arch-specific
* ones such as the O_RDONLY and related macros used by fcntl() and open()
--
2.40.1
Hi Rodrigo,
thanks for your patch!
Some comments below.
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?
Also which is the exact revision you use to extract nolibc?
Does nolibc actually support statx()/stat() on centos-7 with these changes?
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").
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.
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.
> Signed-off-by: Rodrigo Campos <[email protected]>
> ---
> tools/include/nolibc/statx.h | 218 +++++++++++++++++++++++++++++++++++
> tools/include/nolibc/sys.h | 2 +-
> tools/include/nolibc/types.h | 2 +-
> 3 files changed, 220 insertions(+), 2 deletions(-)
> create mode 100644 tools/include/nolibc/statx.h
>
> 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".
> +/*
> + * Compatibility header to allow using statx() on old distros.
> + * Copyright (C) 2023 Rodrigo Campos Catelin <[email protected]>
Assuming copyright for copied code is not great.
> + */
> +
> +#ifndef _NOLIBC_STATX_H
> +#define _NOLIBC_STATX_H
> +
> +/* We should always include this file instead of linux/stat.h, so nolibc works
> + * in old distros too.
> + *
> + * The problem is centos-7, that doesn't have statx() defined in linux/stat.h.
> + * We can't include sys/stat.h because it creates conflicts, so let's just
> + * define it here.
> + * No other distros seem affected by this, so we can remove this file when it
> + * hits EOL (06/2024).
> + */
> +#include <linux/stat.h> /* for statx() */
> +
> +#ifndef STATX_BASIC_STATS
> +
> +/* This is just a c&p from tools/include/uapi/linux/stat.h as it is in
> + * Linux 6.6-rc3.
> + * We don't need it all, but it's easier to just copy it all in case in the
> + * future we start using more of it, as we won't have CI running on centos-7.
> + */
> +#include <linux/types.h>
> +
> +#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
> +
> +#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.
> +
> +#endif
> [..]
> 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.
> #include <linux/prctl.h>
>
> +#include "statx.h" /* for statx() */
> #include "arch.h"
> #include "errno.h"
> #include "types.h"
> [..]
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
On 9/27/23 20:23, Thomas Weißschuh wrote:
> On 2023-09-27 15:06:03+0200, Rodrigo Campos wrote:
>> On 9/27/23 01:30, Thomas Weißschuh wrote:
>>> On 2023-09-26 15:36:47+0200, Rodrigo Campos wrote:
>> 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 :)
>
> How often are you planning on updating your vendoring?
> In the timeframe before you are dropping centos-7 support?
We will probably update if other MIPS variants are added, or other
arches supported by golang. Other than that, I don't see that happening.
> The "nice" thing about the breakage is that it will break loudly during
> compilation so it will be easy to notice and fix it up.
>
>> I don't think it is worth for nolibc, at least for this use case, to
>> reintroduce compatibility for stat() without statx().
>
> It wouldn't even be full compatibility. The code would compile but be
> unusuable for stat()/statx(). And I don't think any application expects
> stat() to return -ENOSYS.
Right, it would not be fully compatible but it will be possible to
compile and use the rest of the syscalls, just not stat().
It's really up to you to decide if that is worth or not. That happens to
be what we need :)
> It's a bit ugly code to support a kernel that has been EOL upstream for
> six years for a fairly specific usecase.
> But who knows, maybe Willy has a soft spot for the 3.10 kernel :-)
> Let's wait for his input.
I can't agree more, that is why I was unsure supporting centos-7 was
something we want to do in the first place.
Let's wait for Willy, but I will be slow to answer in the coming weeks,
I'll be with limited internet connectivity.
Best,
Rodrigo
On 2023-09-27 15:06:03+0200, Rodrigo Campos wrote:
> On 9/27/23 01:30, Thomas Weißschuh wrote:
> > thanks for your patch!
>
> Thank you for nolibc :)
You need to thank Willy for the most part :-)
> > 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 :)
How often are you planning on updating your vendoring?
In the timeframe before you are dropping centos-7 support?
The "nice" thing about the breakage is that it will break loudly during
compilation so it will be easy to notice and fix it up.
> I don't think it is worth for nolibc, at least for this use case, to
> reintroduce compatibility for stat() without statx().
It wouldn't even be full compatibility. The code would compile but be
unusuable for stat()/statx(). And I don't think any application expects
stat() to return -ENOSYS.
It's a bit ugly code to support a kernel that has been EOL upstream for
six years for a fairly specific usecase.
But who knows, maybe Willy has a soft spot for the 3.10 kernel :-)
Let's wait for his input.
> 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)
Without __NR_statx this would still only result in dead code.
*IF* we want to fix/work around this in nolibc it would be more like
#ifdef __NR_statx
/* whatever was done before */
#else
int stat(const char *path, struct stat *buf)
{
return __sysret(-ENOSYS);
}
#endif
Or we drop the #else completely to make it obvious for applications that
stat() will just not work.
> >
> > 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.
Got it.
> > > 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)?
Not entirely sure. Personally I would just try to avoid all of it :-)
> > > +#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.
They are already defined inside types.h of nolibc itself.
So it wouldn't matter if the host has it.
> 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.
Got it.
Hi, Thomas
> On 2023-09-27 15:06:03+0200, Rodrigo Campos wrote:
> ...
> > 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)
>
> Without __NR_statx this would still only result in dead code.
>
> *IF* we want to fix/work around this in nolibc it would be more like
>
> #ifdef __NR_statx
>
> /* whatever was done before */
>
> #else
>
> int stat(const char *path, struct stat *buf)
> {
> return __sysret(-ENOSYS);
> }
>
> #endif
>
> Or we drop the #else completely to make it obvious for applications that
> stat() will just not work.
This may worth a patch, sorry for my fault ;-(
And to avoid breaking the older kernel users or especially for some
older kernel books readers, and since this also has introduced some
inconvenience to Willy, perhaps it is valuable to restore original
'sys_stat' support, I will look into how or simply revert the commit but
need to add a new one for powerpc, I will learn how to avoid defining
the extra structure for the not required architectures, the
__ARCH_WANT_SYS_STAT macro may help. I'm also working on cleaning up the
old_select(), old_mmap() and sys_fork()/sys_clone() selection support,
they may be put in a series.
Willy, what about your suggestion?
Thanks,
Zhangjin Wu