| Message ID | 20231004124712.3833-3-chrubis@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add tst_iterate_fd() | expand |
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 >
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)); ...
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
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)
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(-)