| Message ID | 1c61c673a02152c30edac0e25438257c23c3322e.1581680021.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new LTP tests related to fsmount family of syscalls | expand |
Viresh Kumar <viresh.kumar@linaro.org> wrote: .... > +/* > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > + * > + * Description: > + * Basic fsmount() test. > + */ > +#include "tst_test.h" > +#include "lapi/fsmount.h" > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It seems we have to do this for all the tests involves ‘AT_FDCWD’. + > +static void run(void) > +{ > ... > + > + TEST(fsmount(fd, 0, 0)); > As this fsmount01.c is duplicated with zlang@'s patch, I suggest rewriting an enhancement version maybe name fsmount02.c to cover more fsmount attributes. Since it is named basic fsmount() test, it shouldn't only test fsmount(fd, 0, 0), right? +#define MOUNT_ATTR_RDONLY 0x00000001 /* Mount read-only */ +#define MOUNT_ATTR_NOSUID 0x00000002 /* Ignore suid and sgid bits */ +#define MOUNT_ATTR_NODEV 0x00000004 /* Disallow access to device special files */ +#define MOUNT_ATTR_NOEXEC 0x00000008 /* Disallow program execution */ +#define MOUNT_ATTR__ATIME 0x00000070 /* Setting on how atime should be updated */ +#define MOUNT_ATTR_RELATIME 0x00000000 /* - Update atime relative to mtime/ctime. */ +#define MOUNT_ATTR_NOATIME 0x00000010 /* - Do not update access times. */ +#define MOUNT_ATTR_STRICTATIME 0x00000020 /* - Always perform atime updates */ +#define MOUNT_ATTR_NODIRATIME 0x00000080 /* Do not update directory access times */ > + if (TST_RET == -1) > + tst_brk(TFAIL | TERRNO, "fsmount() failed"); > + > + fsmfd = TST_RET; > + > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > + MOVE_MOUNT_F_EMPTY_PATH)); > + SAFE_CLOSE(fsmfd); > I guess we probably need a way to verify the move_mount() does work. The function ismount() in zlang@'s patch could be extracted into the library as tst_ismount() for all of these tests. @Petr Vorel <pvorel@suse.cz> WDT? > + > + if (TST_RET == -1) > + tst_brk(TBROK | TERRNO, "move_mount() failed"); > + > + SAFE_CLOSE(TST_RET); > + TEST(umount(MNTPOINT)); > SAFE_UMOUNT(MNTPOINT); + > + tst_res(TPASS, "fsmount() passed"); > +} > + > +static struct tst_test test = { > Suggest referring to my previous comments for this part. + .min_kver = "5.2", > + .test_all = run, > + .setup = setup, > + .cleanup = cleanup, > + .needs_root = 1, > + .needs_tmpdir = 1, > + .format_device = 1, > + .mntpoint = MNTPOINT, > +}; >
On 17-02-20, 16:17, Li Wang wrote: > Viresh Kumar <viresh.kumar@linaro.org> wrote: > > .... > > +/* > > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > > + * > > + * Description: > > + * Basic fsmount() test. > > + */ > > +#include "tst_test.h" > > +#include "lapi/fsmount.h" > > > > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It seems > we have to do this for all the tests involves ‘AT_FDCWD’. my fsmount.h header includes <fcntl.h>, won't that be enough ? > + > > +static void run(void) > > +{ > > ... > > + > > + TEST(fsmount(fd, 0, 0)); > > > > As this fsmount01.c is duplicated with zlang@'s patch, I suggest rewriting > an enhancement version maybe name fsmount02.c to cover more fsmount > attributes. Since it is named basic fsmount() test, it shouldn't only test > fsmount(fd, 0, 0), right? > > +#define MOUNT_ATTR_RDONLY 0x00000001 /* Mount read-only */ > +#define MOUNT_ATTR_NOSUID 0x00000002 /* Ignore suid and sgid bits */ > +#define MOUNT_ATTR_NODEV 0x00000004 /* Disallow access to device > special files */ > +#define MOUNT_ATTR_NOEXEC 0x00000008 /* Disallow program execution */ > +#define MOUNT_ATTR__ATIME 0x00000070 /* Setting on how atime should > be updated */ > +#define MOUNT_ATTR_RELATIME 0x00000000 /* - Update atime relative to > mtime/ctime. */ > +#define MOUNT_ATTR_NOATIME 0x00000010 /* - Do not update access times. > */ > +#define MOUNT_ATTR_STRICTATIME 0x00000020 /* - Always perform atime > updates */ > +#define MOUNT_ATTR_NODIRATIME 0x00000080 /* Do not update directory > access times */ Okay, I will give it a try. > > + if (TST_RET == -1) > > + tst_brk(TFAIL | TERRNO, "fsmount() failed"); > > + > > + fsmfd = TST_RET; > > + > > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > > + MOVE_MOUNT_F_EMPTY_PATH)); > > + SAFE_CLOSE(fsmfd); > > > > I guess we probably need a way to verify the move_mount() does work. The > function ismount() in zlang@'s patch could be extracted into the library as > tst_ismount() for all of these tests. > @Petr Vorel <pvorel@suse.cz> WDT? Yeah, I will do that.
On Mon, Feb 17, 2020 at 4:29 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > ... > > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It seems > > we have to do this for all the tests involves ‘AT_FDCWD’. > > my fsmount.h header includes <fcntl.h>, won't that be enough ? > I'm afraid it's not enough. After having a closer look, the reason is that the AT_FDCWD is defined with condition __USE_ATFILE, the __USE_ATFILE depends on _ATFILE_SOURCE defined, and _ATFILE_SOURCE needs enable _GNU_SOURCE. So another effective way is to add '#define _GNU_SOURCE' in front of your test then that <fcntl.h> will be work for you. # rpm -qa glibc-headers glibc-headers-2.5-123 # cat /usr/include/fcntl.h |grep AT_FDCWD -B 2 -A 2 #ifdef __USE_ATFILE # define AT_FDCWD -100 /* Special value used to indicate the *at functions should use the current working directory. */
> On 17-02-20, 16:17, Li Wang wrote: > > Viresh Kumar <viresh.kumar@linaro.org> wrote: > > .... > > > +/* > > > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > > > + * > > > + * Description: > > > + * Basic fsmount() test. > > > + */ > > > +#include "tst_test.h" > > > +#include "lapi/fsmount.h" > > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It seems > > we have to do this for all the tests involves ‘AT_FDCWD’. > my fsmount.h header includes <fcntl.h>, won't that be enough ? Unfortunately it won't be enough. LAPI files are here for adding missing definitions in old distros, which is exactly the case for AT_FDCWD on old RHEL. I also suggest following change to v7, see diff below (fsmount.h using "lapi/fcntl.h", thus not needed in fsmount01.c). > > > +static void run(void) > > > +{ > > > ... > > > + > > > + TEST(fsmount(fd, 0, 0)); > > As this fsmount01.c is duplicated with zlang@'s patch, I suggest rewriting > > an enhancement version maybe name fsmount02.c to cover more fsmount > > attributes. Since it is named basic fsmount() test, it shouldn't only test > > fsmount(fd, 0, 0), right? +1. > > +#define MOUNT_ATTR_RDONLY 0x00000001 /* Mount read-only */ > > +#define MOUNT_ATTR_NOSUID 0x00000002 /* Ignore suid and sgid bits */ > > +#define MOUNT_ATTR_NODEV 0x00000004 /* Disallow access to device > > special files */ > > +#define MOUNT_ATTR_NOEXEC 0x00000008 /* Disallow program execution */ > > +#define MOUNT_ATTR__ATIME 0x00000070 /* Setting on how atime should > > be updated */ > > +#define MOUNT_ATTR_RELATIME 0x00000000 /* - Update atime relative to > > mtime/ctime. */ > > +#define MOUNT_ATTR_NOATIME 0x00000010 /* - Do not update access times. > > */ > > +#define MOUNT_ATTR_STRICTATIME 0x00000020 /* - Always perform atime > > updates */ > > +#define MOUNT_ATTR_NODIRATIME 0x00000080 /* Do not update directory > > access times */ > Okay, I will give it a try. Great thanks! > > > + if (TST_RET == -1) > > > + tst_brk(TFAIL | TERRNO, "fsmount() failed"); > > > + > > > + fsmfd = TST_RET; > > > + > > > + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, > > > + MOVE_MOUNT_F_EMPTY_PATH)); > > > + SAFE_CLOSE(fsmfd); > > I guess we probably need a way to verify the move_mount() does work. The > > function ismount() in zlang@'s patch could be extracted into the library as > > tst_ismount() for all of these tests. > > @Petr Vorel <pvorel@suse.cz> WDT? > Yeah, I will do that. +1. But I'd suggest to first merge tests which I sent as v7 [1] (with change below) and then doing this. Kind regards, Petr [1] https://patchwork.ozlabs.org/project/ltp/list/?series=158902&state=* [2] https://travis-ci.org/pevik/ltp/builds/651390749 diff --git include/lapi/fsmount.h include/lapi/fsmount.h index 87f2f229c..97c41629c 100644 --- include/lapi/fsmount.h +++ include/lapi/fsmount.h @@ -7,12 +7,12 @@ #ifndef FSMOUNT_H__ #define FSMOUNT_H__ -#include <fcntl.h> +#include "config.h" #include <sys/mount.h> #include <sys/syscall.h> #include <sys/types.h> -#include "config.h" +#include "lapi/fcntl.h" #include "lapi/syscalls.h" #ifndef HAVE_FSOPEN diff --git testcases/kernel/syscalls/fsmount/fsmount01.c testcases/kernel/syscalls/fsmount/fsmount01.c index 464458080..6ba226acc 100644 --- testcases/kernel/syscalls/fsmount/fsmount01.c +++ testcases/kernel/syscalls/fsmount/fsmount01.c @@ -10,7 +10,6 @@ #include <sys/mount.h> #include "tst_test.h" -#include "lapi/fcntl.h" #include "lapi/fsmount.h" #include "tst_safe_stdio.h"
Hi Li, > On Mon, Feb 17, 2020 at 4:29 PM Viresh Kumar <viresh.kumar@linaro.org> > wrote: > > ... > > > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It seems > > > we have to do this for all the tests involves ‘AT_FDCWD’. > > my fsmount.h header includes <fcntl.h>, won't that be enough ? > I'm afraid it's not enough. > After having a closer look, the reason is that the AT_FDCWD is defined with > condition __USE_ATFILE, the __USE_ATFILE depends on _ATFILE_SOURCE > defined, and _ATFILE_SOURCE needs enable _GNU_SOURCE. > So another effective way is to add '#define _GNU_SOURCE' in front of your > test then that <fcntl.h> will be work for you. OK, we can add #define _GNU_SOURCE to fsmount01.c, which actually needs it + keep <fcntl.h> there and load lapi/fsmount.h later (see below). But generally this will be the approach for all uses of <fcntl.h> (and probably some other headers) for old distros. IMHO this change is caused by: c941736c92 Remove _BSD_SOURCE and _SVID_SOURCE. (glibc-2.20) c688b41960 Add _DEFAULT_SOURCE feature test macro. (glibc-2.19) So we can either add it to many places or detect this old glibc and compile with -D_GNU_SOURCE (but this might break other things). But that's another story. > # rpm -qa glibc-headers > glibc-headers-2.5-123 > # cat /usr/include/fcntl.h |grep AT_FDCWD -B 2 -A 2 > #ifdef __USE_ATFILE > # define AT_FDCWD -100 /* Special value used to indicate > the *at functions should > use the > current working directory. > */ So is this diff to v7 better? Kind regards, Petr diff --git testcases/kernel/syscalls/fsmount/fsmount01.c testcases/kernel/syscalls/fsmount/fsmount01.c index 464458080..21d0ae50b 100644 --- testcases/kernel/syscalls/fsmount/fsmount01.c +++ testcases/kernel/syscalls/fsmount/fsmount01.c @@ -7,10 +7,11 @@ * to mount a filesystem without any specified mount options. */ +#define _GNU_SOURCE #include <sys/mount.h> +#include <fcntl.h> #include "tst_test.h" -#include "lapi/fcntl.h" #include "lapi/fsmount.h" #include "tst_safe_stdio.h"
On Mon, Feb 17, 2020 at 5:58 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > On Mon, Feb 17, 2020 at 4:29 PM Viresh Kumar <viresh.kumar@linaro.org> > > wrote: > > > > ... > > > > Adding #include "lapi/fcntl.h" in case of ‘AT_FDCWD’ undeclared. It > seems > > > > we have to do this for all the tests involves ‘AT_FDCWD’. > > > > my fsmount.h header includes <fcntl.h>, won't that be enough ? > > > > I'm afraid it's not enough. > > > After having a closer look, the reason is that the AT_FDCWD is defined > with > > condition __USE_ATFILE, the __USE_ATFILE depends on _ATFILE_SOURCE > > defined, and _ATFILE_SOURCE needs enable _GNU_SOURCE. > > > So another effective way is to add '#define _GNU_SOURCE' in front of your > > test then that <fcntl.h> will be work for you. > > OK, we can add #define _GNU_SOURCE to fsmount01.c, which actually needs it > + keep <fcntl.h> there and load lapi/fsmount.h later (see below). > But generally this will be the approach for all uses of <fcntl.h> (and > probably > some other headers) for old distros. IMHO this change is caused by: > > c941736c92 Remove _BSD_SOURCE and _SVID_SOURCE. (glibc-2.20) > c688b41960 Add _DEFAULT_SOURCE feature test macro. (glibc-2.19) > Thanks for figure out this. > > So we can either add it to many places or detect this old glibc and > compile with > -D_GNU_SOURCE (but this might break other things). > -1 remove old Glibc is a bad choice. > But that's another story. > > > # rpm -qa glibc-headers > > glibc-headers-2.5-123 > > > # cat /usr/include/fcntl.h |grep AT_FDCWD -B 2 -A 2 > > #ifdef __USE_ATFILE > > # define AT_FDCWD -100 /* Special value used to indicate > > the *at functions should > > use the > > current working > directory. > > */ > > So is this diff to v7 better? > To use "lapi/fcntl.h" in v7 is simpler I guess, sometimes we just need to get compile pass on old distros but not perform it indeed. And that's the purpose why we need lapi/*.
On Mon, Feb 17, 2020 at 6:28 PM Li Wang <liwang@redhat.com> wrote: > ... >> So we can either add it to many places or detect this old glibc and >> compile with >> -D_GNU_SOURCE (but this might break other things). >> > > -1 remove old Glibc is a bad choice. > Sorry, that's a typo. I mean detect Glibc version is not a good idea:).
Hi Li, > >> So we can either add it to many places or detect this old glibc and > >> compile with > >> -D_GNU_SOURCE (but this might break other things). > > -1 remove old Glibc is a bad choice. > Sorry, that's a typo. I mean detect Glibc version is not a good idea:). Agree. IMHO mostly we should use lapi than libc headers (+ put system headers into lapi files), that'd keep old distros working. Kind regards, Petr
diff --git a/configure.ac b/configure.ac index f006c53e7df1..734fb10c4b31 100644 --- a/configure.ac +++ b/configure.ac @@ -81,6 +81,7 @@ AC_CHECK_FUNCS([ \ fallocate \ fchownat \ fsconfig \ + fsmount \ fsopen \ fstatat \ getdents \ diff --git a/runtest/syscalls b/runtest/syscalls index e27e94d5d17f..0b1def7d4659 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -545,6 +545,9 @@ fanotify15 fanotify15 fsconfig01 fsconfig01 fsconfig02 fsconfig02 +fsmount01 fsmount01 +fsmount02 fsmount02 + fsopen01 fsopen01 fsopen02 fsopen02 diff --git a/testcases/kernel/syscalls/fsmount/.gitignore b/testcases/kernel/syscalls/fsmount/.gitignore new file mode 100644 index 000000000000..aaa66d57f417 --- /dev/null +++ b/testcases/kernel/syscalls/fsmount/.gitignore @@ -0,0 +1,2 @@ +fsmount01 +fsmount02 diff --git a/testcases/kernel/syscalls/fsmount/Makefile b/testcases/kernel/syscalls/fsmount/Makefile new file mode 100644 index 000000000000..5ea7d67db123 --- /dev/null +++ b/testcases/kernel/syscalls/fsmount/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/fsmount/fsmount01.c b/testcases/kernel/syscalls/fsmount/fsmount01.c new file mode 100644 index 000000000000..83fcf0d51f47 --- /dev/null +++ b/testcases/kernel/syscalls/fsmount/fsmount01.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Description: + * Basic fsmount() test. + */ +#include "tst_test.h" +#include "lapi/fsmount.h" + +#define MNTPOINT "mntpoint" + +static int fd, fsmfd; + +static void setup(void) +{ + TEST(fsopen(tst_device->fs_type, 0)); + fd = TST_RET; + + if (fd == -1) + tst_brk(TBROK | TERRNO, "fsopen() failed"); +} + +static void cleanup(void) +{ + SAFE_CLOSE(fd); +} + +static void run(void) +{ + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0)); + if (TST_RET == -1) + tst_brk(TBROK | TERRNO, "fsconfig() failed"); + + TEST(fsconfig(fd, FSCONFIG_SET_FLAG, "rw", NULL, 0)); + if (TST_RET == -1) + tst_brk(TBROK | TERRNO, "fsconfig() failed"); + + TEST(fsconfig(fd, FSCONFIG_CMD_CREATE, NULL, NULL, 0)); + if (TST_RET == -1) + tst_brk(TBROK | TERRNO, "fsconfig() failed"); + + TEST(fsmount(fd, 0, 0)); + if (TST_RET == -1) + tst_brk(TFAIL | TERRNO, "fsmount() failed"); + + fsmfd = TST_RET; + + TEST(move_mount(fsmfd, "", AT_FDCWD, MNTPOINT, + MOVE_MOUNT_F_EMPTY_PATH)); + SAFE_CLOSE(fsmfd); + + if (TST_RET == -1) + tst_brk(TBROK | TERRNO, "move_mount() failed"); + + SAFE_CLOSE(TST_RET); + TEST(umount(MNTPOINT)); + + tst_res(TPASS, "fsmount() passed"); +} + +static struct tst_test test = { + .min_kver = "5.2", + .test_all = run, + .setup = setup, + .cleanup = cleanup, + .needs_root = 1, + .needs_tmpdir = 1, + .format_device = 1, + .mntpoint = MNTPOINT, +}; diff --git a/testcases/kernel/syscalls/fsmount/fsmount02.c b/testcases/kernel/syscalls/fsmount/fsmount02.c new file mode 100644 index 000000000000..cbe99bf99c1f --- /dev/null +++ b/testcases/kernel/syscalls/fsmount/fsmount02.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Description: + * Basic fsmount() failure tests. + */ +#include "tst_test.h" +#include "lapi/fsmount.h" + +int fd, invalid_fd = -1; + +static struct tcase { + char *name; + int *fd; + unsigned int flags; + unsigned int mount_attrs; + int exp_errno; +} tcases[] = { + {"invalid-fd", &invalid_fd, FSMOUNT_CLOEXEC, 0, EBADF}, + {"invalid-flags", &fd, 0x02, 0, EINVAL}, + {"invalid-attrs", &fd, FSMOUNT_CLOEXEC, 0x100, EINVAL}, +}; + +static void cleanup(void) +{ + SAFE_CLOSE(fd); +} + +static void setup(void) +{ + TEST(fsopen(tst_device->fs_type, 0)); + fd = TST_RET; + + if (fd == -1) + tst_brk(TBROK | TERRNO, "fsopen() failed"); + + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0)); + if (TST_RET == -1) + goto out; + + TEST(fsconfig(fd, FSCONFIG_SET_FLAG, "rw", NULL, 0)); + if (TST_RET == -1) + goto out; + + TEST(fsconfig(fd, FSCONFIG_CMD_CREATE, NULL, NULL, 0)); + if (TST_RET != -1) + return; + +out: + cleanup(); + tst_brk(TBROK | TERRNO, "fsconfig() failed"); +} + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + + TEST(fsmount(*tc->fd, tc->flags, tc->mount_attrs)); + if (TST_RET != -1) { + SAFE_CLOSE(TST_RET); + tst_brk(TFAIL, "%s: fsmount() succeeded unexpectedly (index: %d)", + tc->name, n); + } + + if (tc->exp_errno != TST_ERR) { + tst_brk(TFAIL | TTERRNO, "%s: fsmount() should fail with %s", + tc->name, tst_strerrno(tc->exp_errno)); + } + + tst_res(TPASS | TTERRNO, "%s: fsmount() failed as expected", tc->name); +} + +static struct tst_test test = { + .min_kver = "5.2", + .tcnt = ARRAY_SIZE(tcases), + .test = run, + .setup = setup, + .cleanup = cleanup, + .needs_root = 1, + .needs_tmpdir = 1, + .format_device = 1, +};
Add tests to check working of fsmount() syscall. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- configure.ac | 1 + runtest/syscalls | 3 + testcases/kernel/syscalls/fsmount/.gitignore | 2 + testcases/kernel/syscalls/fsmount/Makefile | 6 ++ testcases/kernel/syscalls/fsmount/fsmount01.c | 71 ++++++++++++++++ testcases/kernel/syscalls/fsmount/fsmount02.c | 83 +++++++++++++++++++ 6 files changed, 166 insertions(+) create mode 100644 testcases/kernel/syscalls/fsmount/.gitignore create mode 100644 testcases/kernel/syscalls/fsmount/Makefile create mode 100644 testcases/kernel/syscalls/fsmount/fsmount01.c create mode 100644 testcases/kernel/syscalls/fsmount/fsmount02.c