| Message ID | 20230213183149.231779-1-skhan@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d8e45bf1aed2e5fddd8985b5bb1aaf774a97aba8 |
| Headers | show |
| Series | selftests/mount_setattr: fix redefine struct mount_attr build error | expand |
On Mon, Feb 13, 2023 at 11:31:49AM -0700, Shuah Khan wrote: > Fix the following build error due to redefining struct mount_attr by > removing duplicate define from mount_setattr_test.c > > gcc -g -isystem .../tools/testing/selftests/../../../usr/include -Wall -O2 -pthread mount_setattr_test.c -o .../tools/testing/selftests/mount_setattr/mount_setattr_test > mount_setattr_test.c:107:8: error: redefinition of ‘struct mount_attr’ > 107 | struct mount_attr { > | ^~~~~~~~~~ > In file included from /usr/include/x86_64-linux-gnu/sys/mount.h:32, > from mount_setattr_test.c:10: > .../usr/include/linux/mount.h:129:8: note: originally defined here > 129 | struct mount_attr { > | ^~~~~~~~~~ > make: *** [../lib.mk:145: .../tools/testing/selftests/mount_setattr/mount_setattr_test] Error 1 > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > --- > tools/testing/selftests/mount_setattr/mount_setattr_test.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c > index 8c5fea68ae67..582669ca38e9 100644 > --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c > +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c > @@ -103,13 +103,6 @@ > #else > #define __NR_mount_setattr 442 > #endif > - > -struct mount_attr { > - __u64 attr_set; > - __u64 attr_clr; > - __u64 propagation; > - __u64 userns_fd; > -}; > #endif The difficulty with this is that whether or not you need this definition depends on your system headers. My laptop doesn't have definitions for either __NR_mount_setattr or struct mount_attr, so for me the build works without this patch but fails with it. I suppose we could fix this universally by using a different name for the struct in the test, e.g.: diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c index 8c5fea68ae67..0658129d526e 100644 --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c @@ -103,13 +103,6 @@ #else #define __NR_mount_setattr 442 #endif - -struct mount_attr { - __u64 attr_set; - __u64 attr_clr; - __u64 propagation; - __u64 userns_fd; -}; #endif #ifndef __NR_open_tree @@ -132,6 +125,13 @@ struct mount_attr { #endif #endif +struct __mount_attr { + __u64 attr_set; + __u64 attr_clr; + __u64 propagation; + __u64 userns_fd; +}; + #ifndef MOUNT_ATTR_IDMAP #define MOUNT_ATTR_IDMAP 0x00100000 #endif @@ -141,7 +141,7 @@ struct mount_attr { #endif static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags, - struct mount_attr *attr, size_t size) + struct __mount_attr *attr, size_t size) { return syscall(__NR_mount_setattr, dfd, path, flags, attr, size); } @@ -347,7 +347,7 @@ static bool is_shared_mount(const char *path) static void *mount_setattr_thread(void *data) { - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID, .attr_clr = 0, .propagation = MS_SHARED, @@ -445,7 +445,7 @@ FIXTURE_TEARDOWN(mount_setattr) TEST_F(mount_setattr, invalid_attributes) { - struct mount_attr invalid_attr = { + struct __mount_attr invalid_attr = { .attr_set = (1U << 31), }; @@ -479,11 +479,11 @@ TEST_F(mount_setattr, extensibility) { unsigned int old_flags = 0, new_flags = 0, expected_flags = 0; char *s = "dummy"; - struct mount_attr invalid_attr = {}; + struct __mount_attr invalid_attr = {}; struct mount_attr_large { - struct mount_attr attr1; - struct mount_attr attr2; - struct mount_attr attr3; + struct __mount_attr attr1; + struct __mount_attr attr2; + struct __mount_attr attr3; } large_attr = {}; if (!mount_setattr_supported()) @@ -542,7 +542,7 @@ TEST_F(mount_setattr, extensibility) TEST_F(mount_setattr, basic) { unsigned int old_flags = 0, new_flags = 0, expected_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOEXEC | MOUNT_ATTR_RELATIME, .attr_clr = MOUNT_ATTR__ATIME, }; @@ -578,7 +578,7 @@ TEST_F(mount_setattr, basic_recursive) { int fd; unsigned int old_flags = 0, new_flags = 0, expected_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOEXEC | MOUNT_ATTR_RELATIME, .attr_clr = MOUNT_ATTR__ATIME, }; @@ -672,7 +672,7 @@ TEST_F(mount_setattr, mount_has_writers) { int fd, dfd; unsigned int old_flags = 0, new_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOEXEC | MOUNT_ATTR_RELATIME, .attr_clr = MOUNT_ATTR__ATIME, .propagation = MS_SHARED, @@ -729,7 +729,7 @@ TEST_F(mount_setattr, mount_has_writers) TEST_F(mount_setattr, mixed_mount_options) { unsigned int old_flags1 = 0, old_flags2 = 0, new_flags = 0, expected_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_clr = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NOEXEC | MOUNT_ATTR__ATIME, .attr_set = MOUNT_ATTR_RELATIME, }; @@ -763,7 +763,7 @@ TEST_F(mount_setattr, mixed_mount_options) TEST_F(mount_setattr, time_changes) { unsigned int old_flags = 0, new_flags = 0, expected_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_NODIRATIME | MOUNT_ATTR_NOATIME, }; @@ -967,7 +967,7 @@ TEST_F(mount_setattr, multi_threaded) TEST_F(mount_setattr, wrong_user_namespace) { int ret; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY, }; @@ -983,7 +983,7 @@ TEST_F(mount_setattr, wrong_user_namespace) TEST_F(mount_setattr, wrong_mount_namespace) { int fd, ret; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_RDONLY, }; @@ -1074,7 +1074,7 @@ FIXTURE_TEARDOWN(mount_setattr_idmapped) */ TEST_F(mount_setattr_idmapped, invalid_fd_negative) { - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, .userns_fd = -EBADF, }; @@ -1092,7 +1092,7 @@ TEST_F(mount_setattr_idmapped, invalid_fd_negative) */ TEST_F(mount_setattr_idmapped, invalid_fd_large) { - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, .userns_fd = INT64_MAX, }; @@ -1111,7 +1111,7 @@ TEST_F(mount_setattr_idmapped, invalid_fd_large) TEST_F(mount_setattr_idmapped, invalid_fd_closed) { int fd; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1134,7 +1134,7 @@ TEST_F(mount_setattr_idmapped, invalid_fd_closed) TEST_F(mount_setattr_idmapped, invalid_fd_initial_userns) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1243,7 +1243,7 @@ static int get_userns_fd(unsigned long nsid, unsigned long hostid, unsigned long TEST_F(mount_setattr_idmapped, attached_mount_inside_current_mount_namespace) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1273,7 +1273,7 @@ TEST_F(mount_setattr_idmapped, attached_mount_inside_current_mount_namespace) TEST_F(mount_setattr_idmapped, attached_mount_outside_current_mount_namespace) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1303,7 +1303,7 @@ TEST_F(mount_setattr_idmapped, attached_mount_outside_current_mount_namespace) TEST_F(mount_setattr_idmapped, detached_mount_inside_current_mount_namespace) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1333,7 +1333,7 @@ TEST_F(mount_setattr_idmapped, detached_mount_inside_current_mount_namespace) TEST_F(mount_setattr_idmapped, detached_mount_outside_current_mount_namespace) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1365,7 +1365,7 @@ TEST_F(mount_setattr_idmapped, detached_mount_outside_current_mount_namespace) TEST_F(mount_setattr_idmapped, change_idmapping) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1410,7 +1410,7 @@ static bool expected_uid_gid(int dfd, const char *path, int flags, TEST_F(mount_setattr_idmapped, idmap_mount_tree_invalid) { int open_tree_fd = -EBADF; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP, }; @@ -1445,7 +1445,7 @@ TEST_F(mount_setattr, mount_attr_nosymfollow) { int fd; unsigned int old_flags = 0, new_flags = 0, expected_flags = 0; - struct mount_attr attr = { + struct __mount_attr attr = { .attr_set = MOUNT_ATTR_NOSYMFOLLOW, };
On 2/13/23 16:50, Seth Forshee wrote: > On Mon, Feb 13, 2023 at 11:31:49AM -0700, Shuah Khan wrote: >> Fix the following build error due to redefining struct mount_attr by >> removing duplicate define from mount_setattr_test.c >> >> gcc -g -isystem .../tools/testing/selftests/../../../usr/include -Wall -O2 -pthread mount_setattr_test.c -o .../tools/testing/selftests/mount_setattr/mount_setattr_test >> mount_setattr_test.c:107:8: error: redefinition of ‘struct mount_attr’ >> 107 | struct mount_attr { >> | ^~~~~~~~~~ >> In file included from /usr/include/x86_64-linux-gnu/sys/mount.h:32, >> from mount_setattr_test.c:10: >> .../usr/include/linux/mount.h:129:8: note: originally defined here >> 129 | struct mount_attr { >> | ^~~~~~~~~~ >> make: *** [../lib.mk:145: .../tools/testing/selftests/mount_setattr/mount_setattr_test] Error 1 >> >> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >> --- >> tools/testing/selftests/mount_setattr/mount_setattr_test.c | 7 ------- >> 1 file changed, 7 deletions(-) >> >> diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c >> index 8c5fea68ae67..582669ca38e9 100644 >> --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c >> +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c >> @@ -103,13 +103,6 @@ >> #else >> #define __NR_mount_setattr 442 >> #endif >> - >> -struct mount_attr { >> - __u64 attr_set; >> - __u64 attr_clr; >> - __u64 propagation; >> - __u64 userns_fd; >> -}; >> #endif > > The difficulty with this is that whether or not you need this definition > depends on your system headers. My laptop doesn't have definitions for > either __NR_mount_setattr or struct mount_attr, so for me the build > works without this patch but fails with it. > The header search looks up system headers followed by installed headers in the repo (both in-tree and out-of-tree builds). kselftest builds do depend on headers_install. Did you building after running headers_install? > I suppose we could fix this universally by using a different name for > the struct in the test, e.g.: > This is not a good way to for a couple of reasons. This masks any problems due to incompatibility between these defines. thanks, -- Shuah
On Tue, Feb 14, 2023 at 10:10:00AM -0700, Shuah Khan wrote: > On 2/13/23 16:50, Seth Forshee wrote: > > On Mon, Feb 13, 2023 at 11:31:49AM -0700, Shuah Khan wrote: > > > Fix the following build error due to redefining struct mount_attr by > > > removing duplicate define from mount_setattr_test.c > > > > > > gcc -g -isystem .../tools/testing/selftests/../../../usr/include -Wall -O2 -pthread mount_setattr_test.c -o .../tools/testing/selftests/mount_setattr/mount_setattr_test > > > mount_setattr_test.c:107:8: error: redefinition of ‘struct mount_attr’ > > > 107 | struct mount_attr { > > > | ^~~~~~~~~~ > > > In file included from /usr/include/x86_64-linux-gnu/sys/mount.h:32, > > > from mount_setattr_test.c:10: > > > .../usr/include/linux/mount.h:129:8: note: originally defined here > > > 129 | struct mount_attr { > > > | ^~~~~~~~~~ > > > make: *** [../lib.mk:145: .../tools/testing/selftests/mount_setattr/mount_setattr_test] Error 1 > > > > > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > > > --- > > > tools/testing/selftests/mount_setattr/mount_setattr_test.c | 7 ------- > > > 1 file changed, 7 deletions(-) > > > > > > diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c > > > index 8c5fea68ae67..582669ca38e9 100644 > > > --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c > > > +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c > > > @@ -103,13 +103,6 @@ > > > #else > > > #define __NR_mount_setattr 442 > > > #endif > > > - > > > -struct mount_attr { > > > - __u64 attr_set; > > > - __u64 attr_clr; > > > - __u64 propagation; > > > - __u64 userns_fd; > > > -}; > > > #endif > > > > The difficulty with this is that whether or not you need this definition > > depends on your system headers. My laptop doesn't have definitions for > > either __NR_mount_setattr or struct mount_attr, so for me the build > > works without this patch but fails with it. > > > > The header search looks up system headers followed by installed headers in > the repo (both in-tree and out-of-tree builds). kselftest builds do depend > on headers_install. Did you building after running headers_install? I wasn't aware they depend on headers_install. Why doesn't Documentation/dev-tools/kselftest.rst mention this in the section that describes how to run tests? It seems what I really need to fix the build is to include linux/mount.h, which works for me with or without headers_install, because I have the struct in /usr/include/linux/mount.h. And I suppose the makefile should use KHDR_INCLUDES. So maybe the changes below should also be included. However I know Christian has said that there are challenges with including the mount headers. He wrote this test, so I'd like to hear his thoughts about adding the include. He's on vacation this week though. > > I suppose we could fix this universally by using a different name for > > the struct in the test, e.g.: > > > > This is not a good way to for a couple of reasons. This masks any problems > due to incompatibility between these defines. Agreed that this isn't ideal. Thanks, Seth diff --git a/tools/testing/selftests/mount_setattr/Makefile b/tools/testing/selftests/mount_setattr/Makefile index 2250f7dcb81e..fde72df01b11 100644 --- a/tools/testing/selftests/mount_setattr/Makefile +++ b/tools/testing/selftests/mount_setattr/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for mount selftests. -CFLAGS = -g -I../../../../usr/include/ -Wall -O2 -pthread +CFLAGS = -g $(KHDR_INCLUDES) -Wall -O2 -pthread TEST_GEN_FILES += mount_setattr_test diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c index 8c5fea68ae67..969647228817 100644 --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c @@ -18,6 +18,7 @@ #include <grp.h> #include <stdbool.h> #include <stdarg.h> +#include <linux/mount.h> #include "../kselftest_harness.h"
On 2/14/23 13:45, Seth Forshee wrote: > On Tue, Feb 14, 2023 at 10:10:00AM -0700, Shuah Khan wrote: >>> >> >> The header search looks up system headers followed by installed headers in >> the repo (both in-tree and out-of-tree builds). kselftest builds do depend >> on headers_install. Did you building after running headers_install? > > I wasn't aware they depend on headers_install. Why doesn't > Documentation/dev-tools/kselftest.rst mention this in the section that > describes how to run tests? > It ahs always been a dependency. If you were to compile from the main (root) Makefile as below - headers_install get done before test compile: make kselftest-all TARGETS=mount_setattr > It seems what I really need to fix the build is to include > linux/mount.h, which works for me with or without headers_install, > because I have the struct in /usr/include/linux/mount.h. And I suppose > the makefile should use KHDR_INCLUDES. So maybe the changes below should > also be included. > Yes. Makefile change to use KHDR_INCLUDES is already done. Please take a look at linux-kselftest next - this was done as part of a tree-wide change. If including linux/mount.h is thr correct solution, please send me the patch on top of linux-kselftest next and I will pull it in. > However I know Christian has said that there are challenges with > including the mount headers. He wrote this test, so I'd like to hear his > thoughts about adding the include. He's on vacation this week though. > Sounds good. thanks, -- Shuah
On Tue, Feb 14, 2023 at 04:37:05PM -0700, Shuah Khan wrote: > On 2/14/23 13:45, Seth Forshee wrote: > > On Tue, Feb 14, 2023 at 10:10:00AM -0700, Shuah Khan wrote: > > > > > > > > > > > The header search looks up system headers followed by installed headers in > > > the repo (both in-tree and out-of-tree builds). kselftest builds do depend > > > on headers_install. Did you building after running headers_install? > > > > I wasn't aware they depend on headers_install. Why doesn't > > Documentation/dev-tools/kselftest.rst mention this in the section that > > describes how to run tests? > > > > It ahs always been a dependency. If you were to compile from the > main (root) Makefile as below - headers_install get done before > test compile: > > make kselftest-all TARGETS=mount_setattr > > > It seems what I really need to fix the build is to include > > linux/mount.h, which works for me with or without headers_install, > > because I have the struct in /usr/include/linux/mount.h. And I suppose > > the makefile should use KHDR_INCLUDES. So maybe the changes below should > > also be included. > > > > Yes. Makefile change to use KHDR_INCLUDES is already done. Please > take a look at linux-kselftest next - this was done as part of a > tree-wide change. > > If including linux/mount.h is thr correct solution, please send me > the patch on top of linux-kselftest next and I will pull it in. > > However I know Christian has said that there are challenges with > > including the mount headers. He wrote this test, so I'd like to hear his > > thoughts about adding the include. He's on vacation this week though. The problem is that the linux/mount.h and sys/mount.h headers may conflict depending on the libc version used. So if linux/mount.h is included care needs to be taken that no headers are included that implicitly pull in sys/mount.h and vica versa. But if that isn't a problem in this test and it solves the issue we can just include it.
diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c index 8c5fea68ae67..582669ca38e9 100644 --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c @@ -103,13 +103,6 @@ #else #define __NR_mount_setattr 442 #endif - -struct mount_attr { - __u64 attr_set; - __u64 attr_clr; - __u64 propagation; - __u64 userns_fd; -}; #endif #ifndef __NR_open_tree
Fix the following build error due to redefining struct mount_attr by removing duplicate define from mount_setattr_test.c gcc -g -isystem .../tools/testing/selftests/../../../usr/include -Wall -O2 -pthread mount_setattr_test.c -o .../tools/testing/selftests/mount_setattr/mount_setattr_test mount_setattr_test.c:107:8: error: redefinition of ‘struct mount_attr’ 107 | struct mount_attr { | ^~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/sys/mount.h:32, from mount_setattr_test.c:10: .../usr/include/linux/mount.h:129:8: note: originally defined here 129 | struct mount_attr { | ^~~~~~~~~~ make: *** [../lib.mk:145: .../tools/testing/selftests/mount_setattr/mount_setattr_test] Error 1 Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- tools/testing/selftests/mount_setattr/mount_setattr_test.c | 7 ------- 1 file changed, 7 deletions(-)