Re: [LTP] [PATCH v2 1/4] lib: Add tst_fd iterator

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> centos stream 9 (glibc 2.34)
> https://github.com/pevik/ltp/actions/runs/7415994730/job/20180154319
> In file included from /usr/include/linux/fs.h:19,
>                  from /__w/ltp/ltp/include/lapi/io_uring.h:17,
>                  from /__w/ltp/ltp/lib/tst_fd.c:21:
> /usr/include/x86_64-linux-gnu/sys/mount.h:35:3: error: expected identifier before numeric constant
>    35 |   MS_RDONLY = 1,                /* Mount read-only.  */
>       |   ^~~~~~~~~
> CC lib/tst_fill_file.o
> make[1]: *** [/__w/ltp/ltp/include/mk/rules.mk:15: tst_fd.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> 
> https://sourceware.org/glibc/wiki/Synchronizing_Headers
> does mention conflict between <linux/mount.h> and <sys/mount.h>,
> and that's what happen - <linux/fs.h> includes <linux/mount.h>.
> 
> I send a fix for this which should be applied before the release:
> https://patchwork.ozlabs.org/project/ltp/patch/20240105002914.1463989-1-pvorel@suse.cz/
> 
> It fixes most of the distros:
> https://github.com/pevik/ltp/actions/runs/7416413061/job/20181348475
> 
> But unfortunately it fails on one distro: Ubuntu Bionic (glibc 2.27):
> https://github.com/pevik/ltp/actions/runs/7416413061/job/20181348475
> 
> In file included from ../include/lapi/io_uring.h:17:0,
>                  from tst_fd.c:21:
> /usr/include/x86_64-linux-gnu/sys/mount.h:35:3: error: expected identifier before numeric constant
>    MS_RDONLY = 1,  /* Mount read-only.  */
>    ^
> ../include/mk/rules.mk:15: recipe for target 'tst_fd.o' failed
> 
> I'm not sure if we can fix it. Somebody tried to fix it for QEMU:
> https://lore.kernel.org/qemu-devel/20220802164134.1851910-1-berrange@redhat.com/
> 
> which got later deleted due accepted glibc fix:
> https://lore.kernel.org/qemu-devel/20231109135933.1462615-46-mjt@tls.msk.ru/
> 
> Maybe it's time to drop Ubuntu Bionic? We have Leap 42.2, which is the oldest
> distro we care and it works on it (probably it does not have HAVE_FSOPEN
> defined).
> 
> There is yet another error for very old distros ie. old Leap 42.2 (glibc 2.22),
> probably missing fallback definitions?
> https://github.com/pevik/ltp/actions/runs/7415994730/job/20180153354
> 
> In file included from ../include/lapi/io_uring.h:17:0,
>                  from tst_fd.c:21:
> /usr/include/sys/mount.h:35:3: error: expected identifier before numeric constant
>    MS_RDONLY = 1,  /* Mount read-only.  */
>    ^
> tst_fd.c: In function 'open_io_uring':
> tst_fd.c:195:9: warning: missing initializer for field 'sq_entries' of 'struct io_uring_params' [-Wmissing-field-initializers]
>   struct io_uring_params uring_params = {};
>          ^
> In file included from tst_fd.c:21:0:
> ../include/lapi/io_uring.h:198:11: note: 'sq_entries' declared here
>   uint32_t sq_entries;
>            ^
> tst_fd.c: In function 'open_bpf_map':
> tst_fd.c:208:3: warning: missing initializer for field 'key_size' of 'struct <anonymous>' [-Wmissing-field-initializers]
>    .key_size = 4,
>    ^
> In file included from tst_fd.c:22:0:
> ../include/lapi/bpf.h:185:12: note: 'key_size' declared here
>    uint32_t key_size; /* size of key in bytes */
>             ^
> tst_fd.c:209:3: warning: missing initializer for field 'value_size' of 'struct <anonymous>' [-Wmissing-field-initializers]
>    .value_size = 8,
>    ^
> In file included from tst_fd.c:22:0:
> ../include/lapi/bpf.h:186:12: note: 'value_size' declared here
>    uint32_t value_size; /* size of value in bytes */
>             ^
> tst_fd.c:210:3: warning: missing initializer for field 'max_entries' of 'struct <anonymous>' [-Wmissing-field-initializers]
>    .max_entries = 1,
>    ^
> In file included from tst_fd.c:22:0:
> ../include/lapi/bpf.h:187:12: note: 'max_entries' declared here
>    uint32_t max_entries; /* max number of entries in a map */
>             ^
> tst_fd.c:211:2: warning: missing initializer for field 'map_flags' of 'struct <anonymous>' [-Wmissing-field-initializers]
>   };
>   ^
> In file included from tst_fd.c:22:0:
> ../include/lapi/bpf.h:188:12: note: 'map_flags' declared here
>    uint32_t map_flags; /* BPF_MAP_CREATE related
>             ^
> make[1]: *** [tst_fd.o] Error 1
> ../include/mk/rules.mk:15: recipe for target 'tst_fd.o' failed

Uff, do we still support distros with these header failures?

I especailly used the lapi/ headers where possible in order to avoid any
compilation failures, if lapi/bpf.h fails it's lapi/bpf.h that is broken
though.

> > +static void destroy_pipe(struct tst_fd *fd)
> > +{
> > +	SAFE_CLOSE(fd->priv);
> > +}
> > +
> > +static void open_unix_sock(struct tst_fd *fd)
> > +{
> > +	fd->fd = SAFE_SOCKET(AF_UNIX, SOCK_STREAM, 0);
> > +}
> > +
> > +static void open_inet_sock(struct tst_fd *fd)
> > +{
> > +	fd->fd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> > +}
> > +
> > +static void open_epoll(struct tst_fd *fd)
> > +{
> > +	fd->fd = epoll_create(1);
> > +
> > +	if (fd->fd < 0)
> > +		tst_brk(TBROK | TERRNO, "epoll_create()");
> > +}
> > +
> > +static void open_eventfd(struct tst_fd *fd)
> > +{
> > +	fd->fd = eventfd(0, 0);
> > +
> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> Why there is sometimes TCONF? Permissions? I would expect some check which would
> determine whether TCONF or TBROK. Again, I suppose you'll be able to check, when
> TST_EXP_FAIL() merged, right?

The TCONF branch is added to the calls that can be disabled in kernel.
The CONFIG_EVENTFD can turn off the eventfd() syscall so we can't TBROK
here on a failure.

> > +	}
> > +}
> > +
> > +static void open_signalfd(struct tst_fd *fd)
> > +{
> > +	sigset_t sfd_mask;
> > +	sigemptyset(&sfd_mask);
> > +
> > +	fd->fd = signalfd(-1, &sfd_mask, 0);
> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> > +	}
> > +}
> > +
> > +static void open_timerfd(struct tst_fd *fd)
> > +{
> > +	fd->fd = timerfd_create(CLOCK_REALTIME, 0);
> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> > +	}
> > +}
> > +
> > +static void open_pidfd(struct tst_fd *fd)
> > +{
> > +	fd->fd = pidfd_open(getpid(), 0);
> > +	if (fd->fd < 0)
> > +		tst_brk(TBROK | TERRNO, "pidfd_open()");
> > +}
> > +
> > +static void open_fanotify(struct tst_fd *fd)
> > +{
> > +	fd->fd = fanotify_init(FAN_CLASS_NOTIF, O_RDONLY);
> FYI we have safe_fanotify_init(), which checks for ENOSYS.

But it calls tst_brk() on ENOSYS so we can't use that here.

> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> > +	}
> > +}
> > +
> > +static void open_inotify(struct tst_fd *fd)
> > +{
> > +	fd->fd = inotify_init();
> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> > +	}
> > +}
> > +
> > +static void open_userfaultfd(struct tst_fd *fd)
> > +{
> > +	fd->fd = syscall(__NR_userfaultfd, 0);
> Wouldn't be safe to use tst_syscall() ?

Again that one calls tst_brk() on ENOSYS, we can't call any of the tst_*
or safe_* variants because of that.

> > +
> > +	if (fd->fd < 0) {
> > +		tst_res(TCONF | TERRNO,
> > +			"Skipping %s", tst_fd_desc(fd));
> > +	}
> > +}
> 
> ...
> > +	[TST_FD_FSPICK] = {.open_fd = open_fspick, .desc = "fspick"},
> > +	[TST_FD_OPEN_TREE] = {.open_fd = open_open_tree, .desc = "open_tree"},
> > +	[TST_FD_MEMFD] = {.open_fd = open_memfd, .desc = "memfd"},
> > +	[TST_FD_MEMFD_SECRET] = {.open_fd = open_memfd_secret, .desc = "memfd secret"},
> > +};
> > +
> > +const char *tst_fd_desc(struct tst_fd *fd)
> > +{
> > +	if (fd->type >= ARRAY_SIZE(fd_desc))
> > +		return "invalid";
> Maybe use assert() instead?
> > +
> > +	return fd_desc[fd->type].desc;
> > +}
> > +
> > +void tst_fd_init(struct tst_fd *fd)
> This is not in tst_fd.h, thus check complains about not static.

Ah, right, this is a leftover that should be removed, will do.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp