[PATCH v2 0/4] Add tst_fd iterator API

[PATCH v2 0/4] Add tst_fd iterator API

[Cyril Hrubis]

Changes in v2:

 - Changed the API into iterator rather than a funciton callback
 - Added a lot more fd types
 - Added splice test

Cyril Hrubis (4):
  lib: Add tst_fd iterator
  syscalls: readahead01: Make use of tst_fd
  syscalls: accept: Add tst_fd test
  syscalls: splice07: New splice tst_fd iterator test

 include/tst_fd.h                              |  61 ++++
 include/tst_test.h                            |   1 +
 lib/tst_fd.c                                  | 331 ++++++++++++++++++
 runtest/syscalls                              |   2 +
 testcases/kernel/syscalls/accept/.gitignore   |   1 +
 testcases/kernel/syscalls/accept/accept01.c   |   8 -
 testcases/kernel/syscalls/accept/accept03.c   |  47 +++
 .../kernel/syscalls/readahead/readahead01.c   |  54 +--
 testcases/kernel/syscalls/splice/.gitignore   |   1 +
 testcases/kernel/syscalls/splice/splice07.c   |  85 +++++
 10 files changed, 558 insertions(+), 33 deletions(-)
 create mode 100644 include/tst_fd.h
 create mode 100644 lib/tst_fd.c
 create mode 100644 testcases/kernel/syscalls/accept/accept03.c
 create mode 100644 testcases/kernel/syscalls/splice/splice07.c

-- 
2.41.0

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

[Cyril Hrubis]

Which allows tests to loop over different types of file descriptors

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_fd.h   |  61 +++++++++
 include/tst_test.h |   1 +
 lib/tst_fd.c       | 331 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 393 insertions(+)
 create mode 100644 include/tst_fd.h
 create mode 100644 lib/tst_fd.c

diff --git a/include/tst_fd.h b/include/tst_fd.h
new file mode 100644
index 000000000..2f15a06c8
--- /dev/null
+++ b/include/tst_fd.h
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+#ifndef TST_FD_H__
+#define TST_FD_H__
+
+enum tst_fd_type {
+	TST_FD_FILE,
+	TST_FD_PATH,
+	TST_FD_DIR,
+	TST_FD_DEV_ZERO,
+	TST_FD_PROC_MAPS,
+	TST_FD_PIPE_READ,
+	TST_FD_PIPE_WRITE,
+	TST_FD_UNIX_SOCK,
+	TST_FD_INET_SOCK,
+	TST_FD_EPOLL,
+	TST_FD_EVENTFD,
+	TST_FD_SIGNALFD,
+	TST_FD_TIMERFD,
+	TST_FD_PIDFD,
+	TST_FD_FANOTIFY,
+	TST_FD_INOTIFY,
+	TST_FD_USERFAULTFD,
+	TST_FD_PERF_EVENT,
+	TST_FD_IO_URING,
+	TST_FD_BPF_MAP,
+	TST_FD_FSOPEN,
+	TST_FD_FSPICK,
+	TST_FD_OPEN_TREE,
+	TST_FD_MEMFD,
+	TST_FD_MEMFD_SECRET,
+	TST_FD_MAX,
+};
+
+struct tst_fd {
+	enum tst_fd_type type;
+	int fd;
+	/* used by the library, do not touch! */
+	long priv;
+};
+
+#define TST_FD_INIT {.type = TST_FD_FILE, .fd = -1}
+
+/*
+ * Advances the iterator to the next fd type, returns zero at the end.
+ */
+int tst_fd_next(struct tst_fd *fd);
+
+#define TST_FD_FOREACH(fd) \
+	for (struct tst_fd fd = TST_FD_INIT; tst_fd_next(&fd); )
+
+/*
+ * Returns human readable name for the file descriptor type.
+ */
+const char *tst_fd_desc(struct tst_fd *fd);
+
+#endif /* TST_FD_H__ */
diff --git a/include/tst_test.h b/include/tst_test.h
index 75c2109b9..5eee36bac 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -44,6 +44,7 @@
 #include "tst_taint.h"
 #include "tst_memutils.h"
 #include "tst_arch.h"
+#include "tst_fd.h"
 
 /*
  * Reports testcase result.
diff --git a/lib/tst_fd.c b/lib/tst_fd.c
new file mode 100644
index 000000000..3e0a0fe20
--- /dev/null
+++ b/lib/tst_fd.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+#define TST_NO_DEFAULT_MAIN
+
+#include <sys/epoll.h>
+#include <sys/eventfd.h>
+#include <sys/signalfd.h>
+#include <sys/timerfd.h>
+#include <sys/fanotify.h>
+#include <sys/inotify.h>
+#include <linux/perf_event.h>
+
+#include "tst_test.h"
+#include "tst_safe_macros.h"
+
+#include "lapi/pidfd.h"
+#include "lapi/io_uring.h"
+#include "lapi/bpf.h"
+#include "lapi/fsmount.h"
+
+#include "tst_fd.h"
+
+struct tst_fd_desc {
+	void (*open_fd)(struct tst_fd *fd);
+	void (*destroy)(struct tst_fd *fd);
+	const char *desc;
+};
+
+static void open_file(struct tst_fd *fd)
+{
+	fd->fd = SAFE_OPEN("fd_file", O_RDWR | O_CREAT, 0666);
+	SAFE_UNLINK("fd_file");
+}
+
+static void open_path(struct tst_fd *fd)
+{
+	int tfd;
+
+	tfd = SAFE_CREAT("fd_file", 0666);
+	SAFE_CLOSE(tfd);
+
+	fd->fd = SAFE_OPEN("fd_file", O_PATH);
+
+	SAFE_UNLINK("fd_file");
+}
+
+static void open_dir(struct tst_fd *fd)
+{
+	SAFE_MKDIR("fd_dir", 0700);
+	fd->fd = SAFE_OPEN("fd_dir", O_DIRECTORY);
+	SAFE_RMDIR("fd_dir");
+}
+
+static void open_dev_zero(struct tst_fd *fd)
+{
+	fd->fd = SAFE_OPEN("/dev/zero", O_RDONLY);
+}
+
+static void open_proc_self_maps(struct tst_fd *fd)
+{
+	fd->fd = SAFE_OPEN("/proc/self/maps", O_RDONLY);
+}
+
+static void open_pipe_read(struct tst_fd *fd)
+{
+	int pipe[2];
+
+	SAFE_PIPE(pipe);
+	fd->fd = pipe[0];
+	fd->priv = pipe[1];
+}
+
+static void open_pipe_write(struct tst_fd *fd)
+{
+	int pipe[2];
+
+	SAFE_PIPE(pipe);
+	fd->fd = pipe[1];
+	fd->priv = pipe[0];
+}
+
+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));
+	}
+}
+
+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);
+	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);
+
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_perf_event(struct tst_fd *fd)
+{
+	struct perf_event_attr pe_attr = {
+		.type = PERF_TYPE_SOFTWARE,
+		.size = sizeof(struct perf_event_attr),
+		.config = PERF_COUNT_SW_CPU_CLOCK,
+		.disabled = 1,
+		.exclude_kernel = 1,
+		.exclude_hv = 1,
+	};
+
+	fd->fd = syscall(__NR_perf_event_open, &pe_attr, 0, -1, -1, 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_io_uring(struct tst_fd *fd)
+{
+	struct io_uring_params uring_params = {};
+
+	fd->fd = io_uring_setup(1, &uring_params);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_bpf_map(struct tst_fd *fd)
+{
+	union bpf_attr array_attr = {
+		.map_type = BPF_MAP_TYPE_ARRAY,
+		.key_size = 4,
+		.value_size = 8,
+		.max_entries = 1,
+	};
+
+	fd->fd = bpf(BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_fsopen(struct tst_fd *fd)
+{
+	fd->fd = fsopen("ext2", 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_fspick(struct tst_fd *fd)
+{
+	fd->fd = fspick(AT_FDCWD, "/", 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_open_tree(struct tst_fd *fd)
+{
+	fd->fd = open_tree(AT_FDCWD, "/", 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_memfd(struct tst_fd *fd)
+{
+	fd->fd = syscall(__NR_memfd_create, "ltp_memfd", 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static void open_memfd_secret(struct tst_fd *fd)
+{
+	fd->fd = syscall(__NR_memfd_secret, 0);
+	if (fd->fd < 0) {
+		tst_res(TCONF | TERRNO,
+			"Skipping %s", tst_fd_desc(fd));
+	}
+}
+
+static struct tst_fd_desc fd_desc[] = {
+	[TST_FD_FILE] = {.open_fd = open_file, .desc = "file"},
+	[TST_FD_PATH] = {.open_fd = open_path, .desc = "O_PATH file"},
+	[TST_FD_DIR] = {.open_fd = open_dir, .desc = "directory"},
+	[TST_FD_DEV_ZERO] = {.open_fd = open_dev_zero, .desc = "/dev/zero"},
+	[TST_FD_PROC_MAPS] = {.open_fd = open_proc_self_maps, .desc = "/proc/self/maps"},
+	[TST_FD_PIPE_READ] = {.open_fd = open_pipe_read, .desc = "pipe read end", .destroy = destroy_pipe},
+	[TST_FD_PIPE_WRITE] = {.open_fd = open_pipe_write, .desc = "pipe write end", .destroy = destroy_pipe},
+	[TST_FD_UNIX_SOCK] = {.open_fd = open_unix_sock, .desc = "unix socket"},
+	[TST_FD_INET_SOCK] = {.open_fd = open_inet_sock, .desc = "inet socket"},
+	[TST_FD_EPOLL] = {.open_fd = open_epoll, .desc = "epoll"},
+	[TST_FD_EVENTFD] = {.open_fd = open_eventfd, .desc = "eventfd"},
+	[TST_FD_SIGNALFD] = {.open_fd = open_signalfd, .desc = "signalfd"},
+	[TST_FD_TIMERFD] = {.open_fd = open_timerfd, .desc = "timerfd"},
+	[TST_FD_PIDFD] = {.open_fd = open_pidfd, .desc = "pidfd"},
+	[TST_FD_FANOTIFY] = {.open_fd = open_fanotify, .desc = "fanotify"},
+	[TST_FD_INOTIFY] = {.open_fd = open_inotify, .desc = "inotify"},
+	[TST_FD_USERFAULTFD] = {.open_fd = open_userfaultfd, .desc = "userfaultfd"},
+	[TST_FD_PERF_EVENT] = {.open_fd = open_perf_event, .desc = "perf event"},
+	[TST_FD_IO_URING] = {.open_fd = open_io_uring, .desc = "io uring"},
+	[TST_FD_BPF_MAP] = {.open_fd = open_bpf_map, .desc = "bpf map"},
+	[TST_FD_FSOPEN] = {.open_fd = open_fsopen, .desc = "fsopen"},
+	[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";
+
+	return fd_desc[fd->type].desc;
+}
+
+void tst_fd_init(struct tst_fd *fd)
+{
+	fd->type = TST_FD_FILE;
+	fd->fd = -1;
+}
+
+int tst_fd_next(struct tst_fd *fd)
+{
+	size_t len = ARRAY_SIZE(fd_desc);
+
+	if (fd->fd >= 0) {
+		SAFE_CLOSE(fd->fd);
+
+		if (fd_desc[fd->type].destroy)
+			fd_desc[fd->type].destroy(fd);
+
+		fd->type++;
+	}
+
+	for (;;) {
+		if (fd->type >= len)
+			return 0;
+
+		fd_desc[fd->type].open_fd(fd);
+
+		if (fd->fd >= 0)
+			return 1;
+
+		fd->type++;
+	}
+}
-- 
2.41.0

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

[Richard Palethorpe]

Hello,

Good stuff!

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

Cyril Hrubis <chrubis@suse.cz> writes:

> Which allows tests to loop over different types of file descriptors
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  include/tst_fd.h   |  61 +++++++++
>  include/tst_test.h |   1 +
>  lib/tst_fd.c       | 331 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 393 insertions(+)
>  create mode 100644 include/tst_fd.h
>  create mode 100644 lib/tst_fd.c
>
> diff --git a/include/tst_fd.h b/include/tst_fd.h
> new file mode 100644
> index 000000000..2f15a06c8
> --- /dev/null
> +++ b/include/tst_fd.h
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +#ifndef TST_FD_H__
> +#define TST_FD_H__
> +
> +enum tst_fd_type {
> +	TST_FD_FILE,
> +	TST_FD_PATH,
> +	TST_FD_DIR,
> +	TST_FD_DEV_ZERO,
> +	TST_FD_PROC_MAPS,
> +	TST_FD_PIPE_READ,
> +	TST_FD_PIPE_WRITE,
> +	TST_FD_UNIX_SOCK,
> +	TST_FD_INET_SOCK,
> +	TST_FD_EPOLL,
> +	TST_FD_EVENTFD,
> +	TST_FD_SIGNALFD,
> +	TST_FD_TIMERFD,
> +	TST_FD_PIDFD,
> +	TST_FD_FANOTIFY,
> +	TST_FD_INOTIFY,
> +	TST_FD_USERFAULTFD,
> +	TST_FD_PERF_EVENT,
> +	TST_FD_IO_URING,
> +	TST_FD_BPF_MAP,
> +	TST_FD_FSOPEN,
> +	TST_FD_FSPICK,
> +	TST_FD_OPEN_TREE,
> +	TST_FD_MEMFD,
> +	TST_FD_MEMFD_SECRET,
> +	TST_FD_MAX,
> +};
> +
> +struct tst_fd {
> +	enum tst_fd_type type;
> +	int fd;
> +	/* used by the library, do not touch! */
> +	long priv;
> +};
> +
> +#define TST_FD_INIT {.type = TST_FD_FILE, .fd = -1}
> +
> +/*
> + * Advances the iterator to the next fd type, returns zero at the end.
> + */
> +int tst_fd_next(struct tst_fd *fd);
> +
> +#define TST_FD_FOREACH(fd) \
> +	for (struct tst_fd fd = TST_FD_INIT; tst_fd_next(&fd); )
> +
> +/*
> + * Returns human readable name for the file descriptor type.
> + */
> +const char *tst_fd_desc(struct tst_fd *fd);
> +
> +#endif /* TST_FD_H__ */
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..5eee36bac 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -44,6 +44,7 @@
>  #include "tst_taint.h"
>  #include "tst_memutils.h"
>  #include "tst_arch.h"
> +#include "tst_fd.h"
>  
>  /*
>   * Reports testcase result.
> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> new file mode 100644
> index 000000000..3e0a0fe20
> --- /dev/null
> +++ b/lib/tst_fd.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +
> +#include <sys/epoll.h>
> +#include <sys/eventfd.h>
> +#include <sys/signalfd.h>
> +#include <sys/timerfd.h>
> +#include <sys/fanotify.h>
> +#include <sys/inotify.h>
> +#include <linux/perf_event.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +#include "lapi/pidfd.h"
> +#include "lapi/io_uring.h"
> +#include "lapi/bpf.h"
> +#include "lapi/fsmount.h"
> +
> +#include "tst_fd.h"
> +
> +struct tst_fd_desc {
> +	void (*open_fd)(struct tst_fd *fd);
> +	void (*destroy)(struct tst_fd *fd);
> +	const char *desc;
> +};
> +
> +static void open_file(struct tst_fd *fd)
> +{
> +	fd->fd = SAFE_OPEN("fd_file", O_RDWR | O_CREAT, 0666);
> +	SAFE_UNLINK("fd_file");
> +}
> +
> +static void open_path(struct tst_fd *fd)
> +{
> +	int tfd;
> +
> +	tfd = SAFE_CREAT("fd_file", 0666);
> +	SAFE_CLOSE(tfd);
> +
> +	fd->fd = SAFE_OPEN("fd_file", O_PATH);
> +
> +	SAFE_UNLINK("fd_file");
> +}
> +
> +static void open_dir(struct tst_fd *fd)
> +{
> +	SAFE_MKDIR("fd_dir", 0700);
> +	fd->fd = SAFE_OPEN("fd_dir", O_DIRECTORY);
> +	SAFE_RMDIR("fd_dir");
> +}
> +
> +static void open_dev_zero(struct tst_fd *fd)
> +{
> +	fd->fd = SAFE_OPEN("/dev/zero", O_RDONLY);
> +}
> +
> +static void open_proc_self_maps(struct tst_fd *fd)
> +{
> +	fd->fd = SAFE_OPEN("/proc/self/maps", O_RDONLY);
> +}
> +
> +static void open_pipe_read(struct tst_fd *fd)
> +{
> +	int pipe[2];
> +
> +	SAFE_PIPE(pipe);
> +	fd->fd = pipe[0];
> +	fd->priv = pipe[1];
> +}
> +
> +static void open_pipe_write(struct tst_fd *fd)
> +{
> +	int pipe[2];
> +
> +	SAFE_PIPE(pipe);
> +	fd->fd = pipe[1];
> +	fd->priv = pipe[0];
> +}
> +
> +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));
> +	}
> +}
> +
> +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);
> +	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);
> +
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_perf_event(struct tst_fd *fd)
> +{
> +	struct perf_event_attr pe_attr = {
> +		.type = PERF_TYPE_SOFTWARE,
> +		.size = sizeof(struct perf_event_attr),
> +		.config = PERF_COUNT_SW_CPU_CLOCK,
> +		.disabled = 1,
> +		.exclude_kernel = 1,
> +		.exclude_hv = 1,
> +	};
> +
> +	fd->fd = syscall(__NR_perf_event_open, &pe_attr, 0, -1, -1, 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_io_uring(struct tst_fd *fd)
> +{
> +	struct io_uring_params uring_params = {};
> +
> +	fd->fd = io_uring_setup(1, &uring_params);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_bpf_map(struct tst_fd *fd)
> +{
> +	union bpf_attr array_attr = {
> +		.map_type = BPF_MAP_TYPE_ARRAY,
> +		.key_size = 4,
> +		.value_size = 8,
> +		.max_entries = 1,
> +	};
> +
> +	fd->fd = bpf(BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_fsopen(struct tst_fd *fd)
> +{
> +	fd->fd = fsopen("ext2", 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_fspick(struct tst_fd *fd)
> +{
> +	fd->fd = fspick(AT_FDCWD, "/", 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_open_tree(struct tst_fd *fd)
> +{
> +	fd->fd = open_tree(AT_FDCWD, "/", 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_memfd(struct tst_fd *fd)
> +{
> +	fd->fd = syscall(__NR_memfd_create, "ltp_memfd", 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static void open_memfd_secret(struct tst_fd *fd)
> +{
> +	fd->fd = syscall(__NR_memfd_secret, 0);
> +	if (fd->fd < 0) {
> +		tst_res(TCONF | TERRNO,
> +			"Skipping %s", tst_fd_desc(fd));
> +	}
> +}
> +
> +static struct tst_fd_desc fd_desc[] = {
> +	[TST_FD_FILE] = {.open_fd = open_file, .desc = "file"},
> +	[TST_FD_PATH] = {.open_fd = open_path, .desc = "O_PATH file"},
> +	[TST_FD_DIR] = {.open_fd = open_dir, .desc = "directory"},
> +	[TST_FD_DEV_ZERO] = {.open_fd = open_dev_zero, .desc = "/dev/zero"},
> +	[TST_FD_PROC_MAPS] = {.open_fd = open_proc_self_maps, .desc = "/proc/self/maps"},
> +	[TST_FD_PIPE_READ] = {.open_fd = open_pipe_read, .desc = "pipe read end", .destroy = destroy_pipe},
> +	[TST_FD_PIPE_WRITE] = {.open_fd = open_pipe_write, .desc = "pipe write end", .destroy = destroy_pipe},
> +	[TST_FD_UNIX_SOCK] = {.open_fd = open_unix_sock, .desc = "unix socket"},
> +	[TST_FD_INET_SOCK] = {.open_fd = open_inet_sock, .desc = "inet socket"},
> +	[TST_FD_EPOLL] = {.open_fd = open_epoll, .desc = "epoll"},
> +	[TST_FD_EVENTFD] = {.open_fd = open_eventfd, .desc = "eventfd"},
> +	[TST_FD_SIGNALFD] = {.open_fd = open_signalfd, .desc = "signalfd"},
> +	[TST_FD_TIMERFD] = {.open_fd = open_timerfd, .desc = "timerfd"},
> +	[TST_FD_PIDFD] = {.open_fd = open_pidfd, .desc = "pidfd"},
> +	[TST_FD_FANOTIFY] = {.open_fd = open_fanotify, .desc = "fanotify"},
> +	[TST_FD_INOTIFY] = {.open_fd = open_inotify, .desc = "inotify"},
> +	[TST_FD_USERFAULTFD] = {.open_fd = open_userfaultfd, .desc = "userfaultfd"},
> +	[TST_FD_PERF_EVENT] = {.open_fd = open_perf_event, .desc = "perf event"},
> +	[TST_FD_IO_URING] = {.open_fd = open_io_uring, .desc = "io uring"},
> +	[TST_FD_BPF_MAP] = {.open_fd = open_bpf_map, .desc = "bpf map"},
> +	[TST_FD_FSOPEN] = {.open_fd = open_fsopen, .desc = "fsopen"},
> +	[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";
> +
> +	return fd_desc[fd->type].desc;
> +}
> +
> +void tst_fd_init(struct tst_fd *fd)
> +{
> +	fd->type = TST_FD_FILE;
> +	fd->fd = -1;
> +}
> +
> +int tst_fd_next(struct tst_fd *fd)
> +{
> +	size_t len = ARRAY_SIZE(fd_desc);
> +
> +	if (fd->fd >= 0) {
> +		SAFE_CLOSE(fd->fd);
> +
> +		if (fd_desc[fd->type].destroy)
> +			fd_desc[fd->type].destroy(fd);
> +
> +		fd->type++;
> +	}
> +
> +	for (;;) {
> +		if (fd->type >= len)
> +			return 0;
> +
> +		fd_desc[fd->type].open_fd(fd);
> +
> +		if (fd->fd >= 0)
> +			return 1;
> +
> +		fd->type++;
> +	}
> +}
> -- 
> 2.41.0


-- 
Thank you,
Richard.

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

[Petr Vorel]

Hi Cyril,

> Which allows tests to loop over different types of file descriptors

Nice API, thanks!

> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  include/tst_fd.h   |  61 +++++++++
>  include/tst_test.h |   1 +
>  lib/tst_fd.c       | 331 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 393 insertions(+)
>  create mode 100644 include/tst_fd.h
>  create mode 100644 lib/tst_fd.c

> diff --git a/include/tst_fd.h b/include/tst_fd.h
> new file mode 100644
> index 000000000..2f15a06c8
> --- /dev/null
> +++ b/include/tst_fd.h
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +#ifndef TST_FD_H__
> +#define TST_FD_H__
> +
> +enum tst_fd_type {
> +	TST_FD_FILE,
> +	TST_FD_PATH,
> +	TST_FD_DIR,
> +	TST_FD_DEV_ZERO,
> +	TST_FD_PROC_MAPS,
> +	TST_FD_PIPE_READ,
> +	TST_FD_PIPE_WRITE,
> +	TST_FD_UNIX_SOCK,
> +	TST_FD_INET_SOCK,
> +	TST_FD_EPOLL,
> +	TST_FD_EVENTFD,
> +	TST_FD_SIGNALFD,
> +	TST_FD_TIMERFD,
> +	TST_FD_PIDFD,
> +	TST_FD_FANOTIFY,
> +	TST_FD_INOTIFY,
> +	TST_FD_USERFAULTFD,
> +	TST_FD_PERF_EVENT,
> +	TST_FD_IO_URING,
> +	TST_FD_BPF_MAP,
> +	TST_FD_FSOPEN,
> +	TST_FD_FSPICK,
> +	TST_FD_OPEN_TREE,
> +	TST_FD_MEMFD,
> +	TST_FD_MEMFD_SECRET,
> +	TST_FD_MAX,
> +};
> +
> +struct tst_fd {
> +	enum tst_fd_type type;
> +	int fd;
> +	/* used by the library, do not touch! */
> +	long priv;
> +};
> +
> +#define TST_FD_INIT {.type = TST_FD_FILE, .fd = -1}
> +
> +/*
> + * Advances the iterator to the next fd type, returns zero at the end.
> + */
> +int tst_fd_next(struct tst_fd *fd);
> +
> +#define TST_FD_FOREACH(fd) \
> +	for (struct tst_fd fd = TST_FD_INIT; tst_fd_next(&fd); )
> +
> +/*
> + * Returns human readable name for the file descriptor type.
> + */
> +const char *tst_fd_desc(struct tst_fd *fd);
> +
> +#endif /* TST_FD_H__ */
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..5eee36bac 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -44,6 +44,7 @@
>  #include "tst_taint.h"
>  #include "tst_memutils.h"
>  #include "tst_arch.h"
> +#include "tst_fd.h"

>  /*
>   * Reports testcase result.
> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> new file mode 100644
> index 000000000..3e0a0fe20
> --- /dev/null
> +++ b/lib/tst_fd.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +
> +#include <sys/epoll.h>
> +#include <sys/eventfd.h>
> +#include <sys/signalfd.h>
> +#include <sys/timerfd.h>
> +#include <sys/fanotify.h>
> +#include <sys/inotify.h>
> +#include <linux/perf_event.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +#include "lapi/pidfd.h"
> +#include "lapi/io_uring.h"

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

> +#include "lapi/bpf.h"
> +#include "lapi/fsmount.h"
> +
> +#include "tst_fd.h"

...
> +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?
https://lore.kernel.org/ltp/20240103115700.14585-1-chrubis@suse.cz/

If not, some local macro which would wrap error handling would be useful.

> +	}
> +}
> +
> +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.
> +	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() ?
> +
> +	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.

> +{
> +	fd->type = TST_FD_FILE;
> +	fd->fd = -1;
> +}
...

Kind regards,
Petr

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

[Cyril Hrubis]

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

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

[Petr Vorel]

Hi Cyril,

> > 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?

Unfortunately yes (SLES 12-SP2, somehow covered in CI by openSUSE Leap 42.2).

> 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 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.

OK, thx for info!

Kind regards,
Petr

[PATCH v2 2/4] syscalls: readahead01: Make use of tst_fd

[Cyril Hrubis]

TODO:
- readahead() on /proc/self/maps seems to succeed
- readahead() on pipe write end, O_PATH file and open_tree() fd returns EBADFD

Are these to be expected?

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 .../kernel/syscalls/readahead/readahead01.c   | 54 ++++++++++---------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/testcases/kernel/syscalls/readahead/readahead01.c b/testcases/kernel/syscalls/readahead/readahead01.c
index bdef7945d..6dd5086e5 100644
--- a/testcases/kernel/syscalls/readahead/readahead01.c
+++ b/testcases/kernel/syscalls/readahead/readahead01.c
@@ -30,43 +30,47 @@
 
 static void test_bad_fd(void)
 {
-	char tempname[PATH_MAX] = "readahead01_XXXXXX";
-	int fd;
+	int fd[2];
+
+	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF,
+	             "readahead() with fd = -1");
 
-	tst_res(TINFO, "%s -1", __func__);
-	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF);
+	SAFE_PIPE(fd);
+	SAFE_CLOSE(fd[0]);
+	SAFE_CLOSE(fd[1]);
 
-	tst_res(TINFO, "%s O_WRONLY", __func__);
-	fd = mkstemp(tempname);
-	if (fd == -1)
-		tst_res(TFAIL | TERRNO, "mkstemp failed");
-	SAFE_CLOSE(fd);
-	fd = SAFE_OPEN(tempname, O_WRONLY);
-	TST_EXP_FAIL(readahead(fd, 0, getpagesize()), EBADF);
-	SAFE_CLOSE(fd);
-	unlink(tempname);
+	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EBADF,
+	             "readahead() with invalid fd");
 }
 
-static void test_invalid_fd(void)
+static void test_invalid_fd(struct tst_fd *fd)
 {
-	int fd[2];
+	int exp_errno = EINVAL;
 
-	tst_res(TINFO, "%s pipe", __func__);
-	SAFE_PIPE(fd);
-	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
-	SAFE_CLOSE(fd[0]);
-	SAFE_CLOSE(fd[1]);
+	switch (fd->type) {
+	/* These two succeed */
+	case TST_FD_FILE:
+	case TST_FD_MEMFD:
+		return;
+	case TST_FD_PIPE_WRITE:
+	case TST_FD_OPEN_TREE:
+	case TST_FD_PATH:
+		exp_errno = EBADF;
+	break;
+	default:
+		break;
+	}
 
-	tst_res(TINFO, "%s socket", __func__);
-	fd[0] = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
-	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
-	SAFE_CLOSE(fd[0]);
+	TST_EXP_FAIL(readahead(fd->fd, 0, getpagesize()), exp_errno,
+		     "readahead() on %s", tst_fd_desc(fd));
 }
 
 static void test_readahead(void)
 {
 	test_bad_fd();
-	test_invalid_fd();
+
+	TST_FD_FOREACH(fd)
+		test_invalid_fd(&fd);
 }
 
 static void setup(void)
-- 
2.41.0

Re: [LTP] [PATCH v2 2/4] syscalls: readahead01: Make use of tst_fd

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> TODO:
> - readahead() on /proc/self/maps seems to succeed
> - readahead() on pipe write end, O_PATH file and open_tree() fd returns EBADFD
>
> Are these to be expected?
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  .../kernel/syscalls/readahead/readahead01.c   | 54 ++++++++++---------
>  1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/readahead/readahead01.c b/testcases/kernel/syscalls/readahead/readahead01.c
> index bdef7945d..6dd5086e5 100644
> --- a/testcases/kernel/syscalls/readahead/readahead01.c
> +++ b/testcases/kernel/syscalls/readahead/readahead01.c
> @@ -30,43 +30,47 @@
>  
>  static void test_bad_fd(void)
>  {
> -	char tempname[PATH_MAX] = "readahead01_XXXXXX";
> -	int fd;
> +	int fd[2];
> +
> +	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF,
> +	             "readahead() with fd = -1");
>  
> -	tst_res(TINFO, "%s -1", __func__);
> -	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF);
> +	SAFE_PIPE(fd);
> +	SAFE_CLOSE(fd[0]);
> +	SAFE_CLOSE(fd[1]);

Would it make more sense to just close one of the ends?

Or to open a file with write only?

I wonder whether we still need test_bad_fd at all? Perhaps all the cases
should be integrated into TST_FD_FOREACH?

The rest looks good.

>  
> -	tst_res(TINFO, "%s O_WRONLY", __func__);
> -	fd = mkstemp(tempname);
> -	if (fd == -1)
> -		tst_res(TFAIL | TERRNO, "mkstemp failed");
> -	SAFE_CLOSE(fd);
> -	fd = SAFE_OPEN(tempname, O_WRONLY);
> -	TST_EXP_FAIL(readahead(fd, 0, getpagesize()), EBADF);
> -	SAFE_CLOSE(fd);
> -	unlink(tempname);
> +	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EBADF,
> +	             "readahead() with invalid fd");
>  }
>  
> -static void test_invalid_fd(void)
> +static void test_invalid_fd(struct tst_fd *fd)
>  {
> -	int fd[2];
> +	int exp_errno = EINVAL;
>  
> -	tst_res(TINFO, "%s pipe", __func__);
> -	SAFE_PIPE(fd);
> -	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
> -	SAFE_CLOSE(fd[0]);
> -	SAFE_CLOSE(fd[1]);
> +	switch (fd->type) {
> +	/* These two succeed */
> +	case TST_FD_FILE:
> +	case TST_FD_MEMFD:
> +		return;
> +	case TST_FD_PIPE_WRITE:
> +	case TST_FD_OPEN_TREE:
> +	case TST_FD_PATH:
> +		exp_errno = EBADF;
> +	break;
> +	default:
> +		break;
> +	}
>  
> -	tst_res(TINFO, "%s socket", __func__);
> -	fd[0] = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> -	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
> -	SAFE_CLOSE(fd[0]);
> +	TST_EXP_FAIL(readahead(fd->fd, 0, getpagesize()), exp_errno,
> +		     "readahead() on %s", tst_fd_desc(fd));
>  }
>  
>  static void test_readahead(void)
>  {
>  	test_bad_fd();
> -	test_invalid_fd();
> +
> +	TST_FD_FOREACH(fd)
> +		test_invalid_fd(&fd);
>  }
>  
>  static void setup(void)
> -- 
> 2.41.0


-- 
Thank you,
Richard.

[PATCH v2 3/4] syscalls: accept: Add tst_fd test

[Cyril Hrubis]

It looks like we return wrong errno on O_PATH file and open_tree() file descriptors.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/accept/.gitignore |  1 +
 testcases/kernel/syscalls/accept/accept01.c |  8 ----
 testcases/kernel/syscalls/accept/accept03.c | 47 +++++++++++++++++++++
 4 files changed, 49 insertions(+), 8 deletions(-)
 create mode 100644 testcases/kernel/syscalls/accept/accept03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 53e519639..55396aad8 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -3,6 +3,7 @@ abort01 abort01
 
 accept01 accept01
 accept02 accept02
+accept03 accept03
 
 accept4_01 accept4_01
 
diff --git a/testcases/kernel/syscalls/accept/.gitignore b/testcases/kernel/syscalls/accept/.gitignore
index 5b1462699..f81d4bec9 100644
--- a/testcases/kernel/syscalls/accept/.gitignore
+++ b/testcases/kernel/syscalls/accept/.gitignore
@@ -1,2 +1,3 @@
 /accept01
 /accept02
+/accept03
diff --git a/testcases/kernel/syscalls/accept/accept01.c b/testcases/kernel/syscalls/accept/accept01.c
index 85af0f8af..e5db1dfec 100644
--- a/testcases/kernel/syscalls/accept/accept01.c
+++ b/testcases/kernel/syscalls/accept/accept01.c
@@ -26,7 +26,6 @@
 struct sockaddr_in sin0, sin1, fsin1;
 
 int invalid_socketfd = 400; /* anything that is not an open file */
-int devnull_fd;
 int socket_fd;
 int udp_fd;
 
@@ -45,10 +44,6 @@ static struct test_case {
 		(struct sockaddr *)&fsin1, sizeof(fsin1), EBADF,
 		"bad file descriptor"
 	},
-	{
-		PF_INET, SOCK_STREAM, 0, &devnull_fd, (struct sockaddr *)&fsin1,
-		sizeof(fsin1), ENOTSOCK, "fd is not socket"
-	},
 	{
 		PF_INET, SOCK_STREAM, 0, &socket_fd, (struct sockaddr *)3,
 		sizeof(fsin1), EINVAL, "invalid socket buffer"
@@ -73,8 +68,6 @@ static void test_setup(void)
 	sin0.sin_port = 0;
 	sin0.sin_addr.s_addr = INADDR_ANY;
 
-	devnull_fd = SAFE_OPEN("/dev/null", O_WRONLY);
-
 	socket_fd = SAFE_SOCKET(PF_INET, SOCK_STREAM, 0);
 	SAFE_BIND(socket_fd, (struct sockaddr *)&sin0, sizeof(sin0));
 
@@ -88,7 +81,6 @@ static void test_setup(void)
 
 static void test_cleanup(void)
 {
-	SAFE_CLOSE(devnull_fd);
 	SAFE_CLOSE(socket_fd);
 	SAFE_CLOSE(udp_fd);
 }
diff --git a/testcases/kernel/syscalls/accept/accept03.c b/testcases/kernel/syscalls/accept/accept03.c
new file mode 100644
index 000000000..084bedaf4
--- /dev/null
+++ b/testcases/kernel/syscalls/accept/accept03.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+/*\
+ * [Description]
+ *
+ * Verify that accept() returns ENOTSOCK for non-socket file descriptors.
+ */
+
+#include <sys/socket.h>
+#include <netinet/in.h>
+
+#include "tst_test.h"
+
+void check_accept(struct tst_fd *fd)
+{
+	struct sockaddr_in addr = {
+		.sin_family = AF_INET,
+		.sin_port = 0,
+		.sin_addr = {.s_addr = INADDR_ANY},
+	};
+	socklen_t size = sizeof(addr);
+
+	switch (fd->type) {
+	case TST_FD_UNIX_SOCK:
+	case TST_FD_INET_SOCK:
+		return;
+	default:
+		break;
+	}
+
+	TST_EXP_FAIL2(accept(fd->fd, (void*)&addr, &size),
+		ENOTSOCK, "accept() on %s", tst_fd_desc(fd));
+}
+
+static void verify_accept(void)
+{
+	TST_FD_FOREACH(fd)
+		check_accept(&fd);
+}
+
+static struct tst_test test = {
+	.test_all = verify_accept,
+};
-- 
2.41.0

Re: [LTP] [PATCH v2 3/4] syscalls: accept: Add tst_fd test

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> It looks like we return wrong errno on O_PATH file and open_tree() file descriptors.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  runtest/syscalls                            |  1 +
>  testcases/kernel/syscalls/accept/.gitignore |  1 +
>  testcases/kernel/syscalls/accept/accept01.c |  8 ----
>  testcases/kernel/syscalls/accept/accept03.c | 47 +++++++++++++++++++++
>  4 files changed, 49 insertions(+), 8 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/accept/accept03.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 53e519639..55396aad8 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -3,6 +3,7 @@ abort01 abort01
>  
>  accept01 accept01
>  accept02 accept02
> +accept03 accept03
>  
>  accept4_01 accept4_01
>  
> diff --git a/testcases/kernel/syscalls/accept/.gitignore b/testcases/kernel/syscalls/accept/.gitignore
> index 5b1462699..f81d4bec9 100644
> --- a/testcases/kernel/syscalls/accept/.gitignore
> +++ b/testcases/kernel/syscalls/accept/.gitignore
> @@ -1,2 +1,3 @@
>  /accept01
>  /accept02
> +/accept03
> diff --git a/testcases/kernel/syscalls/accept/accept01.c b/testcases/kernel/syscalls/accept/accept01.c
> index 85af0f8af..e5db1dfec 100644
> --- a/testcases/kernel/syscalls/accept/accept01.c
> +++ b/testcases/kernel/syscalls/accept/accept01.c
> @@ -26,7 +26,6 @@
>  struct sockaddr_in sin0, sin1, fsin1;
>  
>  int invalid_socketfd = 400; /* anything that is not an open file */
> -int devnull_fd;
>  int socket_fd;
>  int udp_fd;
>  
> @@ -45,10 +44,6 @@ static struct test_case {
>  		(struct sockaddr *)&fsin1, sizeof(fsin1), EBADF,
>  		"bad file descriptor"
>  	},
> -	{
> -		PF_INET, SOCK_STREAM, 0, &devnull_fd, (struct sockaddr *)&fsin1,
> -		sizeof(fsin1), ENOTSOCK, "fd is not socket"
> -	},
>  	{
>  		PF_INET, SOCK_STREAM, 0, &socket_fd, (struct sockaddr *)3,
>  		sizeof(fsin1), EINVAL, "invalid socket buffer"
> @@ -73,8 +68,6 @@ static void test_setup(void)
>  	sin0.sin_port = 0;
>  	sin0.sin_addr.s_addr = INADDR_ANY;
>  
> -	devnull_fd = SAFE_OPEN("/dev/null", O_WRONLY);
> -
>  	socket_fd = SAFE_SOCKET(PF_INET, SOCK_STREAM, 0);
>  	SAFE_BIND(socket_fd, (struct sockaddr *)&sin0, sizeof(sin0));
>  
> @@ -88,7 +81,6 @@ static void test_setup(void)
>  
>  static void test_cleanup(void)
>  {
> -	SAFE_CLOSE(devnull_fd);
>  	SAFE_CLOSE(socket_fd);
>  	SAFE_CLOSE(udp_fd);
>  }

Is this supposed to be part of the patchset?

I don't mind, but if we are strict, it should be in another commit.

> diff --git a/testcases/kernel/syscalls/accept/accept03.c b/testcases/kernel/syscalls/accept/accept03.c
> new file mode 100644
> index 000000000..084bedaf4
> --- /dev/null
> +++ b/testcases/kernel/syscalls/accept/accept03.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify that accept() returns ENOTSOCK for non-socket file descriptors.
> + */
> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include "tst_test.h"
> +
> +void check_accept(struct tst_fd *fd)
> +{
> +	struct sockaddr_in addr = {
> +		.sin_family = AF_INET,
> +		.sin_port = 0,
> +		.sin_addr = {.s_addr = INADDR_ANY},
> +	};
> +	socklen_t size = sizeof(addr);
> +
> +	switch (fd->type) {
> +	case TST_FD_UNIX_SOCK:
> +	case TST_FD_INET_SOCK:
> +		return;
> +	default:
> +		break;
> +	}
> +
> +	TST_EXP_FAIL2(accept(fd->fd, (void*)&addr, &size),
> +		ENOTSOCK, "accept() on %s", tst_fd_desc(fd));
> +}
> +
> +static void verify_accept(void)
> +{
> +	TST_FD_FOREACH(fd)
> +		check_accept(&fd);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_accept,
> +};
> -- 
> 2.41.0

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

-- 
Thank you,
Richard.

Re: [LTP] [PATCH v2 3/4] syscalls: accept: Add tst_fd test

[Cyril Hrubis]

Hi!
> >  int invalid_socketfd = 400; /* anything that is not an open file */
> > -int devnull_fd;
> >  int socket_fd;
> >  int udp_fd;
> >  
> > @@ -45,10 +44,6 @@ static struct test_case {
> >  		(struct sockaddr *)&fsin1, sizeof(fsin1), EBADF,
> >  		"bad file descriptor"
> >  	},
> > -	{
> > -		PF_INET, SOCK_STREAM, 0, &devnull_fd, (struct sockaddr *)&fsin1,
> > -		sizeof(fsin1), ENOTSOCK, "fd is not socket"
> > -	},
> >  	{
> >  		PF_INET, SOCK_STREAM, 0, &socket_fd, (struct sockaddr *)3,
> >  		sizeof(fsin1), EINVAL, "invalid socket buffer"
> > @@ -73,8 +68,6 @@ static void test_setup(void)
> >  	sin0.sin_port = 0;
> >  	sin0.sin_addr.s_addr = INADDR_ANY;
> >  
> > -	devnull_fd = SAFE_OPEN("/dev/null", O_WRONLY);
> > -
> >  	socket_fd = SAFE_SOCKET(PF_INET, SOCK_STREAM, 0);
> >  	SAFE_BIND(socket_fd, (struct sockaddr *)&sin0, sizeof(sin0));
> >  
> > @@ -88,7 +81,6 @@ static void test_setup(void)
> >  
> >  static void test_cleanup(void)
> >  {
> > -	SAFE_CLOSE(devnull_fd);
> >  	SAFE_CLOSE(socket_fd);
> >  	SAFE_CLOSE(udp_fd);
> >  }
> 
> Is this supposed to be part of the patchset?
> 
> I don't mind, but if we are strict, it should be in another commit.

That removes ENOTSOCK test that is now handled in accept03, I suppose I
should have explained that better in the comit message.

-- 
Cyril Hrubis
chrubis@suse.cz

[PATCH v2 4/4] syscalls: splice07: New splice tst_fd iterator test

[Cyril Hrubis]

We loop over all possible combinations of file descriptors in the test
and filter out combinations that actually make sense and either block or
attempt to copy data.

The rest of invalid options produce either EINVAL or EBADF and there
does not seem to be any clear pattern to the choices of these two.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/splice/.gitignore |  1 +
 testcases/kernel/syscalls/splice/splice07.c | 85 +++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 testcases/kernel/syscalls/splice/splice07.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 55396aad8..3af634c11 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1515,6 +1515,7 @@ splice03 splice03
 splice04 splice04
 splice05 splice05
 splice06 splice06
+splice07 splice07
 
 tee01 tee01
 tee02 tee02
diff --git a/testcases/kernel/syscalls/splice/.gitignore b/testcases/kernel/syscalls/splice/.gitignore
index 61e979ad6..88a8dff78 100644
--- a/testcases/kernel/syscalls/splice/.gitignore
+++ b/testcases/kernel/syscalls/splice/.gitignore
@@ -4,3 +4,4 @@
 /splice04
 /splice05
 /splice06
+/splice07
diff --git a/testcases/kernel/syscalls/splice/splice07.c b/testcases/kernel/syscalls/splice/splice07.c
new file mode 100644
index 000000000..74d3e9c7a
--- /dev/null
+++ b/testcases/kernel/syscalls/splice/splice07.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+/*\
+ * [Description]
+ *
+ */
+#define _GNU_SOURCE
+
+#include <sys/socket.h>
+#include <netinet/in.h>
+
+#include "tst_test.h"
+
+void check_splice(struct tst_fd *fd_in, struct tst_fd *fd_out)
+{
+	int exp_errno = EINVAL;
+
+	/* These combinations just hang */
+	if (fd_in->type == TST_FD_PIPE_READ) {
+		switch (fd_out->type) {
+		case TST_FD_FILE:
+		case TST_FD_PIPE_WRITE:
+		case TST_FD_UNIX_SOCK:
+		case TST_FD_INET_SOCK:
+		case TST_FD_MEMFD:
+			return;
+		default:
+		break;
+		}
+	}
+
+	if (fd_out->type == TST_FD_PIPE_WRITE) {
+		switch (fd_in->type) {
+		/* While these combinations succeeed */
+		case TST_FD_FILE:
+		case TST_FD_MEMFD:
+			return;
+		/* And this complains about socket not being connected */
+		case TST_FD_INET_SOCK:
+			return;
+		default:
+		break;
+		}
+	}
+
+	/* These produce EBADF instead of EINVAL */
+	switch (fd_out->type) {
+	case TST_FD_DIR:
+	case TST_FD_DEV_ZERO:
+	case TST_FD_PROC_MAPS:
+	case TST_FD_INOTIFY:
+	case TST_FD_PIPE_READ:
+		exp_errno = EBADF;
+	default:
+	break;
+	}
+
+	if (fd_in->type == TST_FD_PIPE_WRITE)
+		exp_errno = EBADF;
+
+	if (fd_in->type == TST_FD_OPEN_TREE || fd_out->type == TST_FD_OPEN_TREE ||
+	    fd_in->type == TST_FD_PATH || fd_out->type == TST_FD_PATH)
+		exp_errno = EBADF;
+
+	TST_EXP_FAIL2(splice(fd_in->fd, NULL, fd_out->fd, NULL, 1, 0),
+		exp_errno, "splice() on %s -> %s",
+		tst_fd_desc(fd_in), tst_fd_desc(fd_out));
+}
+
+static void verify_splice(void)
+{
+	TST_FD_FOREACH(fd_in) {
+		tst_res(TINFO, "%s -> ...", tst_fd_desc(&fd_in));
+		TST_FD_FOREACH(fd_out)
+			check_splice(&fd_in, &fd_out);
+	}
+}
+
+static struct tst_test test = {
+	.test_all = verify_splice,
+};
-- 
2.41.0

Re: [LTP] [PATCH v2 4/4] syscalls: splice07: New splice tst_fd iterator test

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> We loop over all possible combinations of file descriptors in the test
> and filter out combinations that actually make sense and either block or
> attempt to copy data.
>
> The rest of invalid options produce either EINVAL or EBADF and there
> does not seem to be any clear pattern to the choices of these two.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  runtest/syscalls                            |  1 +
>  testcases/kernel/syscalls/splice/.gitignore |  1 +
>  testcases/kernel/syscalls/splice/splice07.c | 85 +++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/splice/splice07.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 55396aad8..3af634c11 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1515,6 +1515,7 @@ splice03 splice03
>  splice04 splice04
>  splice05 splice05
>  splice06 splice06
> +splice07 splice07
>  
>  tee01 tee01
>  tee02 tee02
> diff --git a/testcases/kernel/syscalls/splice/.gitignore b/testcases/kernel/syscalls/splice/.gitignore
> index 61e979ad6..88a8dff78 100644
> --- a/testcases/kernel/syscalls/splice/.gitignore
> +++ b/testcases/kernel/syscalls/splice/.gitignore
> @@ -4,3 +4,4 @@
>  /splice04
>  /splice05
>  /splice06
> +/splice07
> diff --git a/testcases/kernel/syscalls/splice/splice07.c b/testcases/kernel/syscalls/splice/splice07.c
> new file mode 100644
> index 000000000..74d3e9c7a
> --- /dev/null
> +++ b/testcases/kernel/syscalls/splice/splice07.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*\
> + * [Description]
> + *
> + */
> +#define _GNU_SOURCE
> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include "tst_test.h"
> +
> +void check_splice(struct tst_fd *fd_in, struct tst_fd *fd_out)
> +{
> +	int exp_errno = EINVAL;
> +
> +	/* These combinations just hang */

Yup, because there is nothing in the pipe (which you probably realise).

The question is, if we want to test actual splicing, should we fill the
pipe in the lib?

If so should that be an option that we set? TST_FD_FOREACH or
TST_FD_FOREACH2 could take an opts struct for e.g. or even tst_test. I
guess with TST_FD_FOREACH2 there is no need to do add anything now.

> +	if (fd_in->type == TST_FD_PIPE_READ) {
> +		switch (fd_out->type) {
> +		case TST_FD_FILE:
> +		case TST_FD_PIPE_WRITE:
> +		case TST_FD_UNIX_SOCK:
> +		case TST_FD_INET_SOCK:
> +		case TST_FD_MEMFD:
> +			return;
> +		default:
> +		break;
> +		}
> +	}
> +
> +	if (fd_out->type == TST_FD_PIPE_WRITE) {
> +		switch (fd_in->type) {
> +		/* While these combinations succeeed */
> +		case TST_FD_FILE:
> +		case TST_FD_MEMFD:
> +			return;
> +		/* And this complains about socket not being connected */
> +		case TST_FD_INET_SOCK:
> +			return;
> +		default:
> +		break;
> +		}
> +	}
> +
> +	/* These produce EBADF instead of EINVAL */
> +	switch (fd_out->type) {
> +	case TST_FD_DIR:
> +	case TST_FD_DEV_ZERO:
> +	case TST_FD_PROC_MAPS:
> +	case TST_FD_INOTIFY:
> +	case TST_FD_PIPE_READ:
> +		exp_errno = EBADF;
> +	default:
> +	break;
> +	}
> +
> +	if (fd_in->type == TST_FD_PIPE_WRITE)
> +		exp_errno = EBADF;
> +
> +	if (fd_in->type == TST_FD_OPEN_TREE || fd_out->type == TST_FD_OPEN_TREE ||
> +	    fd_in->type == TST_FD_PATH || fd_out->type == TST_FD_PATH)
> +		exp_errno = EBADF;

This seems like something that could change due to checks changing
order.

This is a bit offtopic, but we maybe need errno sets, which would be
useful for our other discussion on relaxing errno checking.

> +
> +	TST_EXP_FAIL2(splice(fd_in->fd, NULL, fd_out->fd, NULL, 1, 0),
> +		exp_errno, "splice() on %s -> %s",
> +		tst_fd_desc(fd_in), tst_fd_desc(fd_out));
> +}
> +
> +static void verify_splice(void)
> +{
> +	TST_FD_FOREACH(fd_in) {
> +		tst_res(TINFO, "%s -> ...", tst_fd_desc(&fd_in));
> +		TST_FD_FOREACH(fd_out)
> +			check_splice(&fd_in, &fd_out);
> +	}

In general test looks great. It turned out clean and simple.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_splice,
> +};
> -- 
> 2.41.0


-- 
Thank you,
Richard.

Re: [LTP] [PATCH v2 4/4] syscalls: splice07: New splice tst_fd iterator test

[Cyril Hrubis]

Hi!
> Yup, because there is nothing in the pipe (which you probably realise).
> 
> The question is, if we want to test actual splicing, should we fill the
> pipe in the lib?
>
> If so should that be an option that we set? TST_FD_FOREACH or
> TST_FD_FOREACH2 could take an opts struct for e.g. or even tst_test. I
> guess with TST_FD_FOREACH2 there is no need to do add anything now.

That would be much more complex. For splicing from a TCP socket I would
have to set up a TCP server, connect the socket there and feed the data
from a sever...

So maybe later on. I would like to avoid adding more complexity to the
patchset at this point and focus on testing errors for now.

> > +	if (fd_in->type == TST_FD_PIPE_READ) {
> > +		switch (fd_out->type) {
> > +		case TST_FD_FILE:
> > +		case TST_FD_PIPE_WRITE:
> > +		case TST_FD_UNIX_SOCK:
> > +		case TST_FD_INET_SOCK:
> > +		case TST_FD_MEMFD:
> > +			return;
> > +		default:
> > +		break;
> > +		}
> > +	}
> > +
> > +	if (fd_out->type == TST_FD_PIPE_WRITE) {
> > +		switch (fd_in->type) {
> > +		/* While these combinations succeeed */
> > +		case TST_FD_FILE:
> > +		case TST_FD_MEMFD:
> > +			return;
> > +		/* And this complains about socket not being connected */
> > +		case TST_FD_INET_SOCK:
> > +			return;
> > +		default:
> > +		break;
> > +		}
> > +	}
> > +
> > +	/* These produce EBADF instead of EINVAL */
> > +	switch (fd_out->type) {
> > +	case TST_FD_DIR:
> > +	case TST_FD_DEV_ZERO:
> > +	case TST_FD_PROC_MAPS:
> > +	case TST_FD_INOTIFY:
> > +	case TST_FD_PIPE_READ:
> > +		exp_errno = EBADF;
> > +	default:
> > +	break;
> > +	}
> > +
> > +	if (fd_in->type == TST_FD_PIPE_WRITE)
> > +		exp_errno = EBADF;
> > +
> > +	if (fd_in->type == TST_FD_OPEN_TREE || fd_out->type == TST_FD_OPEN_TREE ||
> > +	    fd_in->type == TST_FD_PATH || fd_out->type == TST_FD_PATH)
> > +		exp_errno = EBADF;
> 
> This seems like something that could change due to checks changing
> order.

I was hoping that kernel devs would look at the current state, which is
documented in these conditions and tell me how shold we set the
expectations. At least the open_tree() seems to differ from the rest in
several cases, so maybe needs to be aligned with the rest.

> This is a bit offtopic, but we maybe need errno sets, which would be
> useful for our other discussion on relaxing errno checking.

Indeed that is something we have to do either way.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH v2 4/4] syscalls: splice07: New splice tst_fd iterator test

[Jan Kara]

On Tue 24-10-23 09:56:47, Cyril Hrubis wrote:
> > > +	if (fd_in->type == TST_FD_PIPE_READ) {
> > > +		switch (fd_out->type) {
> > > +		case TST_FD_FILE:
> > > +		case TST_FD_PIPE_WRITE:
> > > +		case TST_FD_UNIX_SOCK:
> > > +		case TST_FD_INET_SOCK:
> > > +		case TST_FD_MEMFD:
> > > +			return;
> > > +		default:
> > > +		break;
> > > +		}
> > > +	}
> > > +
> > > +	if (fd_out->type == TST_FD_PIPE_WRITE) {
> > > +		switch (fd_in->type) {
> > > +		/* While these combinations succeeed */
> > > +		case TST_FD_FILE:
> > > +		case TST_FD_MEMFD:
> > > +			return;
> > > +		/* And this complains about socket not being connected */
> > > +		case TST_FD_INET_SOCK:
> > > +			return;
> > > +		default:
> > > +		break;
> > > +		}
> > > +	}
> > > +
> > > +	/* These produce EBADF instead of EINVAL */
> > > +	switch (fd_out->type) {
> > > +	case TST_FD_DIR:
> > > +	case TST_FD_DEV_ZERO:
> > > +	case TST_FD_PROC_MAPS:
> > > +	case TST_FD_INOTIFY:
> > > +	case TST_FD_PIPE_READ:
> > > +		exp_errno = EBADF;
> > > +	default:
> > > +	break;
> > > +	}
> > > +
> > > +	if (fd_in->type == TST_FD_PIPE_WRITE)
> > > +		exp_errno = EBADF;
> > > +
> > > +	if (fd_in->type == TST_FD_OPEN_TREE || fd_out->type == TST_FD_OPEN_TREE ||
> > > +	    fd_in->type == TST_FD_PATH || fd_out->type == TST_FD_PATH)
> > > +		exp_errno = EBADF;
> > 
> > This seems like something that could change due to checks changing
> > order.
> 
> I was hoping that kernel devs would look at the current state, which is
> documented in these conditions and tell me how shold we set the
> expectations. At least the open_tree() seems to differ from the rest in
> several cases, so maybe needs to be aligned with the rest.

Yeah, so the EINVAL vs EBADF vs EISDIR vs ESPIPE distinction is somewhat
arbitrary and as mentioned it very much depends on the order of checks we
do and that is not very consistent among different operations or over
longer time periods. So it would be good if tests could accept all errors
that make some sense. 

E.g. when we cannot seek (change file position) of the fd, ESPIPE is a
valid error return for any operation involving changing file position.
EISDIR is valid error for any directory fd when doing operation not expected
to work on directories. EINVAL and EBADF are quite generic and should be
accepted anytime fd is not suitable for the operation (generally we try to
return EBADF when the descriptor itself isn't suitable - e.g. O_PATH
descriptor, closed descriptor, ... - and return EINVAL when the open
*object* is not suitable but that is a very rough guideline people don't
always follow). EACCES / EPERM should be accepted error return when we
don't have enough permissions to perform operation on the fd. And so on.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [LTP] [PATCH v2 4/4] syscalls: splice07: New splice tst_fd iterator test

[Petr Vorel]

Hi Cyril,

...
> +++ b/testcases/kernel/syscalls/splice/splice07.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*\
> + * [Description]
> + *
nit: missing a description.
> + */
> +#define _GNU_SOURCE
> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include "tst_test.h"
> +
> +void check_splice(struct tst_fd *fd_in, struct tst_fd *fd_out)
nit: missing static

> +	/* These produce EBADF instead of EINVAL */
> +	switch (fd_out->type) {
> +	case TST_FD_DIR:
> +	case TST_FD_DEV_ZERO:
> +	case TST_FD_PROC_MAPS:
> +	case TST_FD_INOTIFY:
> +	case TST_FD_PIPE_READ:
> +		exp_errno = EBADF;
I tested it just on kernel 6.6. I wonder if this behaves the same on older
kernels.

> +	default:
> +	break;
> +	}
> +
> +	if (fd_in->type == TST_FD_PIPE_WRITE)
> +		exp_errno = EBADF;
> +
> +	if (fd_in->type == TST_FD_OPEN_TREE || fd_out->type == TST_FD_OPEN_TREE ||
> +	    fd_in->type == TST_FD_PATH || fd_out->type == TST_FD_PATH)
> +		exp_errno = EBADF;
I suppose you'll send another version, which will make use of TST_EXP_FAIL.
https://lore.kernel.org/ltp/20240103115700.14585-1-chrubis@suse.cz/

BTW I also wonder if TST_EXP_FAIL() could simplify some of fanotify tests
(some of them got quite complex over time).

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr