| Message ID | 495a95969c63d67868b82d2b15bd663f19780d0e.1582779464.git.viresh.kumar@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add new LTP tests related to fsmount family of syscalls | expand |
Hi! > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > + MOVE_MOUNT_F_EMPTY_PATH)); > + > + SAFE_CLOSE(fsmfd); > + > + if (TST_RET == -1) { > + tst_res(TFAIL | TERRNO, "move_mount() failed"); > + goto out; > + } > + > + if (tst_is_mounted(MNTPOINT)) > + tst_res(TPASS, "%s: fsopen() passed", tc->name); > + > + SAFE_UMOUNT(MNTPOINT); I gues sthat the SAFE_UMOUNT() should be inside of the if here and in the rest of the testcases.
On 06-03-20, 14:10, Cyril Hrubis wrote: > Hi! > > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > > + MOVE_MOUNT_F_EMPTY_PATH)); > > + > > + SAFE_CLOSE(fsmfd); > > + > > + if (TST_RET == -1) { > > + tst_res(TFAIL | TERRNO, "move_mount() failed"); > > + goto out; > > + } > > + > > + if (tst_is_mounted(MNTPOINT)) > > + tst_res(TPASS, "%s: fsopen() passed", tc->name); > > + > > + SAFE_UMOUNT(MNTPOINT); > > I gues sthat the SAFE_UMOUNT() should be inside of the if here and in > the rest of the testcases. Petr had a similar comment earlier and here is my explanation to it. There should always be a unmount() in response to a successful call to mount() APIs. What if, because of some other bugs in the kernel or testsuite, tst_is_mounted() returns 0? We should still do the unmount() part as the mount() API didn't return an error.
Hi Viresh, > > > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > > > + MOVE_MOUNT_F_EMPTY_PATH)); > > > + > > > + SAFE_CLOSE(fsmfd); > > > + > > > + if (TST_RET == -1) { > > > + tst_res(TFAIL | TERRNO, "move_mount() failed"); > > > + goto out; > > > + } > > > + > > > + if (tst_is_mounted(MNTPOINT)) > > > + tst_res(TPASS, "%s: fsopen() passed", tc->name); > > > + > > > + SAFE_UMOUNT(MNTPOINT); > > I gues sthat the SAFE_UMOUNT() should be inside of the if here and in > > the rest of the testcases. > Petr had a similar comment earlier and here is my explanation to it. > There should always be a unmount() in response to a successful call to > mount() APIs. What if, because of some other bugs in the kernel or > testsuite, tst_is_mounted() returns 0? We should still do the > unmount() part as the mount() API didn't return an error. But IMHO if device is not mounted we get TBROK due EINVAL in safe_umount(). I'd understand if this was in cleanup function where TBROK turns to TWARN. Kind regards, Petr
On 12-03-20, 09:11, Petr Vorel wrote: > Hi Viresh, > > > > > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > > > > + MOVE_MOUNT_F_EMPTY_PATH)); > > > > + > > > > + SAFE_CLOSE(fsmfd); > > > > + > > > > + if (TST_RET == -1) { > > > > + tst_res(TFAIL | TERRNO, "move_mount() failed"); > > > > + goto out; > > > > + } > > > > + > > > > + if (tst_is_mounted(MNTPOINT)) > > > > + tst_res(TPASS, "%s: fsopen() passed", tc->name); > > > > + > > > > + SAFE_UMOUNT(MNTPOINT); > > > > I gues sthat the SAFE_UMOUNT() should be inside of the if here and in > > > the rest of the testcases. > > > Petr had a similar comment earlier and here is my explanation to it. > > > There should always be a unmount() in response to a successful call to > > mount() APIs. What if, because of some other bugs in the kernel or > > testsuite, tst_is_mounted() returns 0? We should still do the > > unmount() part as the mount() API didn't return an error. > But IMHO if device is not mounted we get TBROK due EINVAL in safe_umount(). But why won't move_mount() fail if device isn't mounted ? Why do we need the tst_is_mounted() helper at all ? Just to catch a case where move_mount() is buggy and doesn't report the failure properly, right ? In case of that bug, isn't it possible that move_mount() allocates some resources which must be freed with a call to umount() nevertheless ?
Hi! > > > > I gues sthat the SAFE_UMOUNT() should be inside of the if here and in > > > > the rest of the testcases. > > > > > Petr had a similar comment earlier and here is my explanation to it. > > > > > There should always be a unmount() in response to a successful call to > > > mount() APIs. What if, because of some other bugs in the kernel or > > > testsuite, tst_is_mounted() returns 0? We should still do the > > > unmount() part as the mount() API didn't return an error. > > But IMHO if device is not mounted we get TBROK due EINVAL in safe_umount(). > > But why won't move_mount() fail if device isn't mounted ? Why do we > need the tst_is_mounted() helper at all ? Just to catch a case where > move_mount() is buggy and doesn't report the failure properly, right ? > In case of that bug, isn't it possible that move_mount() allocates > some resources which must be freed with a call to umount() > nevertheless ? We can't really clean things up when something in kernel is misbehaving. If there is a bug we enter the wast lands of undefined behavior where anything is possible and any attempt to restore the system in a defined state is doomed to fail anyways. So in the end I do not care here as long as we clean up properly when things work as expected, so either way is fine. It only looks a bit strange when we attempt to umount things that are possibly not mounted.
diff --git a/runtest/syscalls b/runtest/syscalls index f51456b8fb53..1f21cc55bf2d 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -343,6 +343,9 @@ fremovexattr02 fremovexattr02 fsmount01 fsmount01 +fsopen01 fsopen01 +fsopen02 fsopen02 + fstat02 fstat02 fstat02_64 fstat02_64 fstat03 fstat03 diff --git a/testcases/kernel/syscalls/fsopen/.gitignore b/testcases/kernel/syscalls/fsopen/.gitignore new file mode 100644 index 000000000000..5da868621883 --- /dev/null +++ b/testcases/kernel/syscalls/fsopen/.gitignore @@ -0,0 +1,2 @@ +/fsopen01 +/fsopen02 diff --git a/testcases/kernel/syscalls/fsopen/Makefile b/testcases/kernel/syscalls/fsopen/Makefile new file mode 100644 index 000000000000..5ea7d67db123 --- /dev/null +++ b/testcases/kernel/syscalls/fsopen/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/fsopen/fsopen01.c b/testcases/kernel/syscalls/fsopen/fsopen01.c new file mode 100644 index 000000000000..abf5f15c4721 --- /dev/null +++ b/testcases/kernel/syscalls/fsopen/fsopen01.c @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Basic fsopen() test which tries to configure and mount the filesystem as + * well. + */ +#include "tst_test.h" +#include "lapi/fsmount.h" + +#define MNTPOINT "mntpoint" + +#define TCASE_ENTRY(_flags) {.name = "Flag " #_flags, .flags = _flags} + +static struct tcase { + char *name; + unsigned int flags; +} tcases[] = { + TCASE_ENTRY(0), + TCASE_ENTRY(FSOPEN_CLOEXEC), +}; + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + int fd, fsmfd; + + TEST(fd = fsopen(tst_device->fs_type, tc->flags)); + if (fd == -1) { + tst_res(TFAIL | TERRNO, "fsopen() failed"); + return; + } + + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0)); + if (TST_RET == -1) { + tst_res(TFAIL | TERRNO, "fsconfig() failed"); + goto out; + } + + TEST(fsconfig(fd, FSCONFIG_CMD_CREATE, NULL, NULL, 0)); + if (TST_RET == -1) { + tst_res(TFAIL | TERRNO, "fsconfig() failed"); + goto out; + } + + TEST(fsmfd = fsmount(fd, 0, 0)); + if (fsmfd == -1) { + tst_res(TFAIL | TERRNO, "fsmount() failed"); + goto out; + } + + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, + MOVE_MOUNT_F_EMPTY_PATH)); + + SAFE_CLOSE(fsmfd); + + if (TST_RET == -1) { + tst_res(TFAIL | TERRNO, "move_mount() failed"); + goto out; + } + + if (tst_is_mounted(MNTPOINT)) + tst_res(TPASS, "%s: fsopen() passed", tc->name); + + SAFE_UMOUNT(MNTPOINT); + +out: + SAFE_CLOSE(fd); +} + +static struct tst_test test = { + .tcnt = ARRAY_SIZE(tcases), + .test = run, + .setup = fsopen_supported_by_kernel, + .needs_root = 1, + .format_device = 1, + .mntpoint = MNTPOINT, + .all_filesystems = 1, + .dev_fs_flags = TST_FS_SKIP_FUSE, +}; diff --git a/testcases/kernel/syscalls/fsopen/fsopen02.c b/testcases/kernel/syscalls/fsopen/fsopen02.c new file mode 100644 index 000000000000..3f287bf2962b --- /dev/null +++ b/testcases/kernel/syscalls/fsopen/fsopen02.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Basic fsopen() failure tests. + */ +#include "tst_test.h" +#include "lapi/fsmount.h" + +const char *invalid_fs = "invalid"; +const char *valid_fs; + +static struct tcase { + char *name; + const char **fs; + unsigned int flags; + int exp_errno; +} tcases[] = { + {"invalid-fs", &invalid_fs, 0, ENODEV}, + {"invalid-flags", &valid_fs, 0x10, EINVAL}, +}; + +static void setup(void) +{ + fsopen_supported_by_kernel(); + + valid_fs = tst_device->fs_type; +} + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + + TEST(fsopen(*tc->fs, tc->flags)); + + if (TST_RET != -1) { + SAFE_CLOSE(TST_RET); + tst_res(TFAIL, "%s: fsopen() succeeded unexpectedly (index: %d)", + tc->name, n); + return; + } + + if (tc->exp_errno != TST_ERR) { + tst_res(TFAIL | TTERRNO, "%s: fsopen() should fail with %s", + tc->name, tst_strerrno(tc->exp_errno)); + return; + } + + tst_res(TPASS | TTERRNO, "%s: fsopen() failed as expected", tc->name); +} + +static struct tst_test test = { + .tcnt = ARRAY_SIZE(tcases), + .test = run, + .setup = setup, + .needs_root = 1, + .needs_device = 1, +};