| Message ID | 20231016123320.9865-5-chrubis@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add tst_fd iterator API | expand |
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
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.
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
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
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, +};
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