[PATCH 0/3] Add tst_iterate_fd()

[PATCH 0/3] Add tst_iterate_fd()

[Cyril Hrubis]

 - adds tst_iterate_fd() functionality
 - make use of tst_iterate_fd() in readahead01
 - add accept03 test which uses tst_iterate_fd()

This is a prototype for how the functionality to iterate over different
file descriptors should look like it converts one tests and adds
another. There is plenty of other syscalls that can use this kind of
testing, e.g. all fooat() syscalls where we can pass invalid dir_fd, the
plan is to add these if/once we agree on the API.

Cyril Hrubis (3):
  lib: Add tst_fd_iterate()
  syscalls/readahead01: Make use of tst_fd_iterate()
  syscalls: accept: Add tst_fd_iterate() test

 include/tst_fd.h                              |  39 ++++++
 include/tst_test.h                            |   1 +
 lib/tst_fd.c                                  | 116 ++++++++++++++++++
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/accept/.gitignore   |   1 +
 testcases/kernel/syscalls/accept/accept01.c   |   8 --
 testcases/kernel/syscalls/accept/accept03.c   |  46 +++++++
 .../kernel/syscalls/readahead/readahead01.c   |  46 +++----
 8 files changed, 224 insertions(+), 34 deletions(-)
 create mode 100644 include/tst_fd.h
 create mode 100644 lib/tst_fd.c
 create mode 100644 testcases/kernel/syscalls/accept/accept03.c

-- 
2.41.0

[PATCH 1/3] lib: Add tst_fd_iterate()

[Cyril Hrubis]

Which allows us to call a function on bunch of different file
descriptors.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_fd.h   |  39 +++++++++++++++
 include/tst_test.h |   1 +
 lib/tst_fd.c       | 116 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 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..711e043dd
--- /dev/null
+++ b/include/tst_fd.h
@@ -0,0 +1,39 @@
+// 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_DIR,
+	TST_FD_DEV_ZERO,
+	TST_FD_PROC_MAPS,
+	TST_FD_PIPE_IN,
+	TST_FD_PIPE_OUT,
+	TST_FD_UNIX_SOCK,
+	TST_FD_INET_SOCK,
+	TST_FD_IO_URING,
+	TST_FD_BPF_MAP,
+	TST_FD_MAX,
+};
+
+struct tst_fd {
+	enum tst_fd_type type;
+	int fd;
+};
+
+/*
+ * Iterates over all fd types and calls the run_test function for each of them.
+ */
+void tst_fd_iterate(void (*run_test)(struct tst_fd *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..7b6cb767e
--- /dev/null
+++ b/lib/tst_fd.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+#define TST_NO_DEFAULT_MAIN
+
+#include "tst_test.h"
+#include "tst_safe_macros.h"
+#include "lapi/io_uring.h"
+#include "lapi/bpf.h"
+
+#include "tst_fd.h"
+
+const char *tst_fd_desc(struct tst_fd *fd)
+{
+	switch (fd->type) {
+	case TST_FD_FILE:
+		return "regular file";
+	case TST_FD_DIR:
+		return "directory";
+	case TST_FD_DEV_ZERO:
+		return "/dev/zero";
+	case TST_FD_PROC_MAPS:
+		return "/proc/self/maps";
+	case TST_FD_PIPE_IN:
+		return "pipe read end";
+	case TST_FD_PIPE_OUT:
+		return "pipe write end";
+	case TST_FD_UNIX_SOCK:
+		return "unix socket";
+	case TST_FD_INET_SOCK:
+		return "inet socket";
+	case TST_FD_IO_URING:
+		return "io_uring";
+	case TST_FD_BPF_MAP:
+		return "bpf map";
+	case TST_FD_MAX:
+	break;
+	}
+
+	return "invalid";
+}
+
+void tst_fd_iterate(void (*run_test)(struct tst_fd *fd))
+{
+	enum tst_fd_type i;
+	struct tst_fd fd;
+	int pipe[2];
+	struct io_uring_params uring_params = {};
+	union bpf_attr array_attr = {
+		.map_type = BPF_MAP_TYPE_ARRAY,
+		.key_size = 4,
+		.value_size = 8,
+		.max_entries = 1,
+	};
+
+	SAFE_PIPE(pipe);
+
+	for (i = 0; i < TST_FD_MAX; i++) {
+		fd.type = i;
+
+		switch (i) {
+		case TST_FD_FILE:
+			fd.fd = SAFE_OPEN("fd_file", O_RDWR | O_CREAT);
+			SAFE_UNLINK("fd_file");
+		break;
+		case TST_FD_DIR:
+			SAFE_MKDIR("fd_dir", 0700);
+			fd.fd = SAFE_OPEN("fd_dir", O_DIRECTORY);
+			SAFE_RMDIR("fd_dir");
+		break;
+		case TST_FD_DEV_ZERO:
+			fd.fd = SAFE_OPEN("/dev/zero", O_RDONLY);
+		break;
+		case TST_FD_PROC_MAPS:
+			fd.fd = SAFE_OPEN("/proc/self/maps", O_RDONLY);
+		break;
+		case TST_FD_PIPE_IN:
+			fd.fd = pipe[0];
+		break;
+		case TST_FD_PIPE_OUT:
+			fd.fd = pipe[1];
+		break;
+		case TST_FD_UNIX_SOCK:
+			fd.fd = SAFE_SOCKET(AF_UNIX, SOCK_STREAM, 0);
+		break;
+		case TST_FD_INET_SOCK:
+			fd.fd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
+		break;
+		case TST_FD_IO_URING:
+			fd.fd = io_uring_setup(1, &uring_params);
+			if (fd.fd < 0) {
+				tst_res(TCONF | TERRNO,
+					"Skipping %s", tst_fd_desc(&fd));
+				continue;
+			}
+		break;
+		case TST_FD_BPF_MAP:
+			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));
+				continue;
+			}
+		break;
+		case TST_FD_MAX:
+		break;
+		}
+
+		run_test(&fd);
+
+		SAFE_CLOSE(fd.fd);
+	}
+}
-- 
2.41.0

Re: [PATCH 1/3] lib: Add tst_fd_iterate()

[Matthew Wilcox]

On Wed, Oct 04, 2023 at 02:47:10PM +0200, Cyril Hrubis wrote:
> +enum tst_fd_type {
> +	TST_FD_FILE,
> +	TST_FD_DIR,
> +	TST_FD_DEV_ZERO,
> +	TST_FD_PROC_MAPS,
> +	TST_FD_PIPE_IN,
> +	TST_FD_PIPE_OUT,
> +	TST_FD_UNIX_SOCK,
> +	TST_FD_INET_SOCK,
> +	TST_FD_IO_URING,
> +	TST_FD_BPF_MAP,
> +	TST_FD_MAX,
> +};

This looks great!  Thanks for turning my musing into concrete code.
Some other file descriptor types that might be interesting ...

O_PATH (see openat(2); some variants on this like opening a symlink with
O_NOFOLLOW)

epoll
eventfd
signalfd
timerfd_create
pidfd_open
fanotify_init
inotify
userfaultfd
perf_event_open
fsopen
fspick
fsmount
open_tree
secretmem
memfd

(i used a variety of techniques for thinking of these, including
grepping for CLOEXEC and fd_install)

Re: [LTP] [PATCH 1/3] lib: Add tst_fd_iterate()

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Which allows us to call a function on bunch of different file
> descriptors.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  include/tst_fd.h   |  39 +++++++++++++++
>  include/tst_test.h |   1 +
>  lib/tst_fd.c       | 116 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 156 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..711e043dd
> --- /dev/null
> +++ b/include/tst_fd.h
> @@ -0,0 +1,39 @@
> +// 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_DIR,
> +	TST_FD_DEV_ZERO,
> +	TST_FD_PROC_MAPS,
> +	TST_FD_PIPE_IN,
> +	TST_FD_PIPE_OUT,
> +	TST_FD_UNIX_SOCK,
> +	TST_FD_INET_SOCK,
> +	TST_FD_IO_URING,
> +	TST_FD_BPF_MAP,
> +	TST_FD_MAX,
> +};
> +
> +struct tst_fd {
> +	enum tst_fd_type type;
> +	int fd;
> +};
> +
> +/*
> + * Iterates over all fd types and calls the run_test function for each of them.
> + */
> +void tst_fd_iterate(void (*run_test)(struct tst_fd *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..7b6cb767e
> --- /dev/null
> +++ b/lib/tst_fd.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2023 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +#include "lapi/io_uring.h"
> +#include "lapi/bpf.h"
> +
> +#include "tst_fd.h"
> +
> +const char *tst_fd_desc(struct tst_fd *fd)
> +{
> +	switch (fd->type) {
> +	case TST_FD_FILE:
> +		return "regular file";
> +	case TST_FD_DIR:
> +		return "directory";
> +	case TST_FD_DEV_ZERO:
> +		return "/dev/zero";
> +	case TST_FD_PROC_MAPS:
> +		return "/proc/self/maps";
> +	case TST_FD_PIPE_IN:
> +		return "pipe read end";
> +	case TST_FD_PIPE_OUT:
> +		return "pipe write end";
> +	case TST_FD_UNIX_SOCK:
> +		return "unix socket";
> +	case TST_FD_INET_SOCK:
> +		return "inet socket";
> +	case TST_FD_IO_URING:
> +		return "io_uring";
> +	case TST_FD_BPF_MAP:
> +		return "bpf map";
> +	case TST_FD_MAX:
> +	break;
> +	}
> +
> +	return "invalid";
> +}
> +
> +void tst_fd_iterate(void (*run_test)(struct tst_fd *fd))
> +{
> +	enum tst_fd_type i;
> +	struct tst_fd fd;
> +	int pipe[2];
> +	struct io_uring_params uring_params = {};
> +	union bpf_attr array_attr = {
> +		.map_type = BPF_MAP_TYPE_ARRAY,
> +		.key_size = 4,
> +		.value_size = 8,
> +		.max_entries = 1,
> +	};
> +
> +	SAFE_PIPE(pipe);
> +
> +	for (i = 0; i < TST_FD_MAX; i++) {
> +		fd.type = i;
> +
> +		switch (i) {
> +		case TST_FD_FILE:
> +			fd.fd = SAFE_OPEN("fd_file", O_RDWR | O_CREAT);
> +			SAFE_UNLINK("fd_file");
> +		break;
> +		case TST_FD_DIR:
> +			SAFE_MKDIR("fd_dir", 0700);
> +			fd.fd = SAFE_OPEN("fd_dir", O_DIRECTORY);
> +			SAFE_RMDIR("fd_dir");
> +		break;
> +		case TST_FD_DEV_ZERO:
> +			fd.fd = SAFE_OPEN("/dev/zero", O_RDONLY);
> +		break;
> +		case TST_FD_PROC_MAPS:
> +			fd.fd = SAFE_OPEN("/proc/self/maps", O_RDONLY);
> +		break;
> +		case TST_FD_PIPE_IN:
> +			fd.fd = pipe[0];
> +		break;
> +		case TST_FD_PIPE_OUT:
> +			fd.fd = pipe[1];
> +		break;
> +		case TST_FD_UNIX_SOCK:
> +			fd.fd = SAFE_SOCKET(AF_UNIX, SOCK_STREAM, 0);
> +		break;
> +		case TST_FD_INET_SOCK:
> +			fd.fd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> +		break;
> +		case TST_FD_IO_URING:
> +			fd.fd = io_uring_setup(1, &uring_params);
> +			if (fd.fd < 0) {
> +				tst_res(TCONF | TERRNO,
> +					"Skipping %s", tst_fd_desc(&fd));
> +				continue;
> +			}
> +		break;
> +		case TST_FD_BPF_MAP:
> +			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));
> +				continue;
> +			}
> +		break;

I don't wish to over complicate this, but how many potential fd types
could there be 100, 1000? Some could have complicated init logic.

I'm wondering if at the outset it would be better to define an interface
struct with name, setup and teardown for each FD type, plus whatever
other meta-data might be useful for filtering.

Then instead of a case statement, we put the structs in an array etc.


> +		case TST_FD_MAX:
> +		break;
> +		}
> +
> +		run_test(&fd);
> +
> +		SAFE_CLOSE(fd.fd);
> +	}
> +}
> -- 
> 2.41.0


-- 
Thank you,
Richard.

Re: [LTP] [PATCH 1/3] lib: Add tst_fd_iterate()

[Cyril Hrubis]

Hi!
> I don't wish to over complicate this, but how many potential fd types
> could there be 100, 1000? Some could have complicated init logic.

I'm at 25 at the moment, suprisingly all of them so far are a syscall
with a few parameters, sometimes packed in a struct.

> I'm wondering if at the outset it would be better to define an interface
> struct with name, setup and teardown for each FD type, plus whatever
> other meta-data might be useful for filtering.
> 
> Then instead of a case statement, we put the structs in an array etc.

I guess that we can, but we would have to add some private data area to
the tst_fd, so that we can tear down things cleanly, but we would need
that if we want to convert the tst_iterate_fd() to be iterator-like
anyways.

-- 
Cyril Hrubis
chrubis@suse.cz

[PATCH 2/3] syscalls/readahead01: Make use of tst_fd_iterate()

[Cyril Hrubis]

TODO: readahead() on /proc/self/maps seems to succeed is that to be
      expected?

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

diff --git a/testcases/kernel/syscalls/readahead/readahead01.c b/testcases/kernel/syscalls/readahead/readahead01.c
index bdef7945d..28134d416 100644
--- a/testcases/kernel/syscalls/readahead/readahead01.c
+++ b/testcases/kernel/syscalls/readahead/readahead01.c
@@ -29,44 +29,38 @@
 #if defined(__NR_readahead)
 
 static void test_bad_fd(void)
-{
-	char tempname[PATH_MAX] = "readahead01_XXXXXX";
-	int fd;
-
-	tst_res(TINFO, "%s -1", __func__);
-	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF);
-
-	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);
-}
-
-static void test_invalid_fd(void)
 {
 	int fd[2];
 
-	tst_res(TINFO, "%s pipe", __func__);
+	TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF,
+	             "readahead() with fd = -1");
+
 	SAFE_PIPE(fd);
-	TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
 	SAFE_CLOSE(fd[0]);
 	SAFE_CLOSE(fd[1]);
 
-	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[0], 0, getpagesize()), EBADF,
+	             "readahead() with invalid fd");
+}
+
+static void test_invalid_fd(struct tst_fd *fd)
+{
+	switch (fd->type) {
+	case TST_FD_FILE:
+	case TST_FD_PIPE_OUT:
+		return;
+	default:
+		break;
+	}
+
+	TST_EXP_FAIL(readahead(fd->fd, 0, getpagesize()), EINVAL,
+		     "readahead() on %s", tst_fd_desc(fd));
 }
 
 static void test_readahead(void)
 {
 	test_bad_fd();
-	test_invalid_fd();
+	tst_fd_iterate(test_invalid_fd);
 }
 
 static void setup(void)
-- 
2.41.0

Re: [PATCH 2/3] syscalls/readahead01: Make use of tst_fd_iterate()

[Amir Goldstein]

On Wed, Oct 4, 2023 at 3:46 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>

Hi Cyril,

Thanks for following up on this!

> TODO: readahead() on /proc/self/maps seems to succeed is that to be
>       expected?

Not sure.
How does llseek() work on the same fd?
Matthew suggested that we align the behavior of both readahead(2)
and posix_fadvise(2) to that of llseek(2)

>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  .../kernel/syscalls/readahead/readahead01.c   | 46 ++++++++-----------
>  1 file changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/readahead/readahead01.c b/testcases/kernel/syscalls/readahead/readahead01.c
> index bdef7945d..28134d416 100644
> --- a/testcases/kernel/syscalls/readahead/readahead01.c
> +++ b/testcases/kernel/syscalls/readahead/readahead01.c
> @@ -29,44 +29,38 @@
>  #if defined(__NR_readahead)
>
>  static void test_bad_fd(void)
> -{
> -       char tempname[PATH_MAX] = "readahead01_XXXXXX";
> -       int fd;
> -
> -       tst_res(TINFO, "%s -1", __func__);
> -       TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF);
> -
> -       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);
> -}
> -
> -static void test_invalid_fd(void)
>  {
>         int fd[2];
>
> -       tst_res(TINFO, "%s pipe", __func__);
> +       TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF,
> +                    "readahead() with fd = -1");
> +

Any reason not to include a bad and a closed fd in the iterator?

>         SAFE_PIPE(fd);
> -       TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
>         SAFE_CLOSE(fd[0]);
>         SAFE_CLOSE(fd[1]);
>
> -       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[0], 0, getpagesize()), EBADF,
> +                    "readahead() with invalid fd");
> +}
> +
> +static void test_invalid_fd(struct tst_fd *fd)
> +{
> +       switch (fd->type) {
> +       case TST_FD_FILE:
> +       case TST_FD_PIPE_OUT:
> +               return;
> +       default:
> +               break;
> +       }
> +
> +       TST_EXP_FAIL(readahead(fd->fd, 0, getpagesize()), EINVAL,
> +                    "readahead() on %s", tst_fd_desc(fd));

Thinking forward and we would like to change this error code to ESPIPE
is there already a helper to expect one of a few error codes?

Thanks,
Amir.

>  }
>
>  static void test_readahead(void)
>  {
>         test_bad_fd();
> -       test_invalid_fd();
> +       tst_fd_iterate(test_invalid_fd);
>  }
>
>  static void setup(void)
> --
> 2.41.0
>

Re: [PATCH 2/3] syscalls/readahead01: Make use of tst_fd_iterate()

[Cyril Hrubis]

Hi!
> > TODO: readahead() on /proc/self/maps seems to succeed is that to be
> >       expected?
> 
> Not sure.
> How does llseek() work on the same fd?

Looks like we we can seek in that file as well, accordingly to man pages
we cannot seek in pipe, socket, and fifo, which seems to match the
reality.  We can apparently seek in O_DIRECTORY fd as well, not sure if
that is even useful.

> > -static void test_invalid_fd(void)
> >  {
> >         int fd[2];
> >
> > -       tst_res(TINFO, "%s pipe", __func__);
> > +       TST_EXP_FAIL(readahead(-1, 0, getpagesize()), EBADF,
> > +                    "readahead() with fd = -1");
> > +
> 
> Any reason not to include a bad and a closed fd in the iterator?

I wanted to avoid mixing valid and invalid fds because we tend to get
different errnos for these, since the situation is different between
"this is not a file descriptor" and "this is not supported on this kind
of file descriptor".

> >         SAFE_PIPE(fd);
> > -       TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
> >         SAFE_CLOSE(fd[0]);
> >         SAFE_CLOSE(fd[1]);
> >
> > -       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[0], 0, getpagesize()), EBADF,
> > +                    "readahead() with invalid fd");
> > +}
> > +
> > +static void test_invalid_fd(struct tst_fd *fd)
> > +{
> > +       switch (fd->type) {
> > +       case TST_FD_FILE:
> > +       case TST_FD_PIPE_OUT:
> > +               return;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       TST_EXP_FAIL(readahead(fd->fd, 0, getpagesize()), EINVAL,
> > +                    "readahead() on %s", tst_fd_desc(fd));
> 
> Thinking forward and we would like to change this error code to ESPIPE
> is there already a helper to expect one of a few error codes?

Not yet. The hardest part is again figuring out right API. We usually
try to check for the new behavior on newer kernels, which would be
complex to encode into the parameters, so maybe we just need to pass a
callback that would return the right errno. Maybe something as:

static int exp_errno(void)
{
	if (tst_kvercmp(6, 7, 0) >= 0)
		return ESPIPE;

	return EINVAL;
}

...
	TST_EXP_FAIL_CB(readahead(fd->fd, 0, getpagesize()), exp_errno,
		"readahead() on %s", tst_fd_desc(fd));
...

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 2/3] syscalls/readahead01: Make use of tst_fd_iterate()

[Jan Kara]

On Wed 04-10-23 16:24:30, Cyril Hrubis wrote:
> Hi!
> > > TODO: readahead() on /proc/self/maps seems to succeed is that to be
> > >       expected?
> > 
> > Not sure.
> > How does llseek() work on the same fd?
> 
> Looks like we we can seek in that file as well, accordingly to man pages
> we cannot seek in pipe, socket, and fifo, which seems to match the
> reality.  We can apparently seek in O_DIRECTORY fd as well, not sure if
> that is even useful.

Seeking on O_DIRECTORY fd is actually well defined by POSIX. You can store
current file position you've got back from lseek and you are guaranteed to
get back at the same position in the directory if you lseek to it (even if
there was a change to the directory, although effects on changed directory
entries is undefined). This is actually pretty tough to implement in
contemporary filesystems with non-trivial directory structure but that is
how POSIX defined it...

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

[PATCH 3/3] syscalls: accept: Add tst_fd_iterate() test

[Cyril Hrubis]

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 | 46 +++++++++++++++++++++
 4 files changed, 48 insertions(+), 8 deletions(-)
 create mode 100644 testcases/kernel/syscalls/accept/accept03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 8652e0bd3..25b53a724 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..6bced33b6
--- /dev/null
+++ b/testcases/kernel/syscalls/accept/accept03.c
@@ -0,0 +1,46 @@
+// 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_iterate(check_accept);
+}
+
+static struct tst_test test = {
+	.test_all = verify_accept,
+};
-- 
2.41.0

Re: [LTP] [PATCH 0/3] Add tst_iterate_fd()

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

>  - adds tst_iterate_fd() functionality
>  - make use of tst_iterate_fd() in readahead01
>  - add accept03 test which uses tst_iterate_fd()
>
> This is a prototype for how the functionality to iterate over different
> file descriptors should look like it converts one tests and adds
> another. There is plenty of other syscalls that can use this kind of
> testing, e.g. all fooat() syscalls where we can pass invalid dir_fd, the
> plan is to add these if/once we agree on the API.

I imagine the results of using this with splice could be very interesting.

>
> Cyril Hrubis (3):
>   lib: Add tst_fd_iterate()
>   syscalls/readahead01: Make use of tst_fd_iterate()
>   syscalls: accept: Add tst_fd_iterate() test
>
>  include/tst_fd.h                              |  39 ++++++
>  include/tst_test.h                            |   1 +
>  lib/tst_fd.c                                  | 116 ++++++++++++++++++
>  runtest/syscalls                              |   1 +
>  testcases/kernel/syscalls/accept/.gitignore   |   1 +
>  testcases/kernel/syscalls/accept/accept01.c   |   8 --
>  testcases/kernel/syscalls/accept/accept03.c   |  46 +++++++
>  .../kernel/syscalls/readahead/readahead01.c   |  46 +++----
>  8 files changed, 224 insertions(+), 34 deletions(-)
>  create mode 100644 include/tst_fd.h
>  create mode 100644 lib/tst_fd.c
>  create mode 100644 testcases/kernel/syscalls/accept/accept03.c
>
> -- 
> 2.41.0


-- 
Thank you,
Richard.

Re: [LTP] [PATCH 0/3] Add tst_iterate_fd()

[Cyril Hrubis]

Hi!
> >  - adds tst_iterate_fd() functionality
> >  - make use of tst_iterate_fd() in readahead01
> >  - add accept03 test which uses tst_iterate_fd()
> >
> > This is a prototype for how the functionality to iterate over different
> > file descriptors should look like it converts one tests and adds
> > another. There is plenty of other syscalls that can use this kind of
> > testing, e.g. all fooat() syscalls where we can pass invalid dir_fd, the
> > plan is to add these if/once we agree on the API.
> 
> I imagine the results of using this with splice could be very interesting.

Good idea, I guess that we need to figure out how to do carthesian
multiplication on the different file descriptors though. Maybe we need
to treat the tst_interate_fd() as an iterator so that we can advance to
the next fd with each call, so that we can do:

	struct tst_fd fd_in = {}, fd_out = {};

	while (tst_iterate_fd(&fd_in)) {
		while (tst_iterate_fd(&fd_out)) {
			...
			TST_TEST(splice(fd_in.fd, 0, fd_out.fd, 0, ...));
			...
		}
	}

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 0/3] Add tst_iterate_fd()

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> >  - adds tst_iterate_fd() functionality
>> >  - make use of tst_iterate_fd() in readahead01
>> >  - add accept03 test which uses tst_iterate_fd()
>> >
>> > This is a prototype for how the functionality to iterate over different
>> > file descriptors should look like it converts one tests and adds
>> > another. There is plenty of other syscalls that can use this kind of
>> > testing, e.g. all fooat() syscalls where we can pass invalid dir_fd, the
>> > plan is to add these if/once we agree on the API.
>> 
>> I imagine the results of using this with splice could be very interesting.
>
> Good idea, I guess that we need to figure out how to do carthesian
> multiplication on the different file descriptors though. Maybe we need
> to treat the tst_interate_fd() as an iterator so that we can advance to
> the next fd with each call, so that we can do:
>
> 	struct tst_fd fd_in = {}, fd_out = {};
>
> 	while (tst_iterate_fd(&fd_in)) {
> 		while (tst_iterate_fd(&fd_out)) {
> 			...
> 			TST_TEST(splice(fd_in.fd, 0, fd_out.fd, 0, ...));
> 			...
> 		}
> 	}

This looks promising. I think it would be good to try this sooner rather
than later.

-- 
Thank you,
Richard.

Re: [LTP] [PATCH 0/3] Add tst_iterate_fd()

[Cyril Hrubis]

Hi!
> This looks promising. I think it would be good to try this sooner rather
> than later.

Already rewriting the code...

-- 
Cyril Hrubis
chrubis@suse.cz