| Message ID | 20230226192554.669332-1-fontaine.fabrice@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | configure.ac: fix mount_attr detection | expand |
Hi! > Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with > latest glibc which provides mount_attr in sys/mount.h. Unfortunately, > the following build failure is still raised because sys/mount is now > unconditionally included in include/lapi/fsmount.h: > > In file included from fsconfig01.c:9: > ../../../../include/lapi/fsmount.h:55:8: error: redefinition of 'struct mount_attr' > 55 | struct mount_attr { > | ^~~~~~~~~~ > In file included from ../../../../include/lapi/fsmount.h:14: > /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: note: originally defined here > 210 | struct mount_attr > | ^~~~~~~~~~ > > Fixes: > - http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > --- > configure.ac | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/configure.ac b/configure.ac > index c2b0f48e7..a6d8ac826 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struc > > AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) > AC_CHECK_TYPES([struct mount_attr],,,[ > -#ifdef HAVE_LINUX_MOUNT_H > -# include <linux/mount.h> > -#else > +#ifdef HAVE_MOUNT_SETATTR > # include <sys/mount.h> > +#elif HAVE_LINUX_MOUNT_H > +# include <linux/mount.h> > #endif > ]) I wonder if we can get this whole mess of two different fallback headers simplified. Looking at the glibc implementation it seems to include "linux/mount.h" if it does exist. So most reasonable solution would do the same I guess which we did before the commit you reference. @Li Wang where does the the sys/mount.h and linux/mount.h conflict? As far as I can tell the UAPI linux/mount.h never defined any functions, just constants, which should be solvable without the complex ifdefs, we just need to check for existence of the types and define them only if missing.
Hi all, [ Cc Li - question from Cyril at the end ] Fabrice, thanks for fixing this long standing issue. > Hi! > > Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with > > latest glibc which provides mount_attr in sys/mount.h. Unfortunately, > > the following build failure is still raised because sys/mount is now > > unconditionally included in include/lapi/fsmount.h: > > In file included from fsconfig01.c:9: > > ../../../../include/lapi/fsmount.h:55:8: error: redefinition of 'struct mount_attr' > > 55 | struct mount_attr { > > | ^~~~~~~~~~ > > In file included from ../../../../include/lapi/fsmount.h:14: > > /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: note: originally defined here > > 210 | struct mount_attr > > | ^~~~~~~~~~ > > Fixes: > > - http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > > --- > > configure.ac | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/configure.ac b/configure.ac > > index c2b0f48e7..a6d8ac826 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struc > > AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) > > AC_CHECK_TYPES([struct mount_attr],,,[ > > -#ifdef HAVE_LINUX_MOUNT_H > > -# include <linux/mount.h> > > -#else > > +#ifdef HAVE_MOUNT_SETATTR > > # include <sys/mount.h> > > +#elif HAVE_LINUX_MOUNT_H > > +# include <linux/mount.h> > > #endif > > ]) > I wonder if we can get this whole mess of two different fallback headers > simplified. Looking at the glibc implementation it seems to include > "linux/mount.h" if it does exist. So most reasonable solution would do > the same I guess which we did before the commit you reference. > @Li Wang where does the the sys/mount.h and linux/mount.h conflict? As > far as I can tell the UAPI linux/mount.h never defined any functions, > just constants, which should be solvable without the complex ifdefs, we > just need to check for existence of the types and define them only if > missing. Quoting [1]: The newer Glibc already provided wrapper for the series pidfd syscall, so let's include the header file conditionally. # rpm -q glibc-devel glibc-devel-2.35.9000-31.fc37.ppc64le # rpm -ql glibc-devel | grep pidfd /usr/include/sys/pidfd.h To get rid of compiling error from fedora-rawhide: tst_safe_macros.c: In function ‘safe_pidfd_open’: tst_safe_macros.c:135:16: error: implicit declaration of function ‘pidfd_open’ [-Werror=implicit-function-declaration] 135 | rval = pidfd_open(pid, flags); | => Li, does the problem still persists? If yes, I'd be for merging this patch. Could you also test it on newest glibc in Fedora? Cyril, FYI musl never includes linux headers (Rich Felker has always been against this practise). Kind regards, Petr [1] https://lore.kernel.org/ltp/20220805063401.1647479-2-liwang@redhat.com/
On Mon, Feb 27, 2023 at 7:26 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with > > latest glibc which provides mount_attr in sys/mount.h. Unfortunately, > > the following build failure is still raised because sys/mount is now > > unconditionally included in include/lapi/fsmount.h: > > > > In file included from fsconfig01.c:9: > > ../../../../include/lapi/fsmount.h:55:8: error: redefinition of 'struct > mount_attr' > > 55 | struct mount_attr { > > | ^~~~~~~~~~ > > In file included from ../../../../include/lapi/fsmount.h:14: > > > /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: > note: originally defined here > > 210 | struct mount_attr > > | ^~~~~~~~~~ > > > > Fixes: > > - > http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 > > > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > > --- > > configure.ac | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > index c2b0f48e7..a6d8ac826 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, > struct __kernel_old_timespec, struc > > > > AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) > > AC_CHECK_TYPES([struct mount_attr],,,[ > > -#ifdef HAVE_LINUX_MOUNT_H > > -# include <linux/mount.h> > > -#else > > +#ifdef HAVE_MOUNT_SETATTR > > # include <sys/mount.h> > > +#elif HAVE_LINUX_MOUNT_H > > +# include <linux/mount.h> > > #endif > > ]) > > I wonder if we can get this whole mess of two different fallback headers > simplified. Looking at the glibc implementation it seems to include > "linux/mount.h" if it does exist. So most reasonable solution would do > the same I guess which we did before the commit you reference. > > @Li Wang where does the the sys/mount.h and linux/mount.h conflict? As > I remember in the newer Glibc that added new definitions (e.g. mount_setattr, fsopen) support in sys/mount.h which conflict with linux/mount.h. That's the reason I submit the patch to resolve it for some middle version of Glibc exists the problem. But that problem later resolves in Glibc as well by: https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 Check my Fedora37: --------------------------- $ rpm -qf /usr/include/sys/mount.h glibc-headers-x86-2.34-49.fc35.noarch $ grep mount_attr /usr/include/sys/mount.h #define MOUNT_ATTR_IDMAP 0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */ struct mount_attr struct mount_attr *__uattr, size_t __usize) $ rpm -qf /usr/include/linux/mount.h kernel-headers-6.1.5-200.fc37.x86_64 $ grep mount_attr /usr/include/linux/mount.h #define MOUNT_ATTR_IDMAP 0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */ struct mount_attr { /* List of all mount_attr versions. */ far as I can tell the UAPI linux/mount.h never defined any functions, > just constants, which should be solvable without the complex ifdefs, we > just need to check for existence of the types and define them only if > missing. > > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
On Thu, Mar 9, 2023 at 6:56 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi all, > > [ Cc Li - question from Cyril at the end ] > > Fabrice, thanks for fixing this long standing issue. > > > Hi! > > > Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with > > > latest glibc which provides mount_attr in sys/mount.h. Unfortunately, > > > the following build failure is still raised because sys/mount is now > > > unconditionally included in include/lapi/fsmount.h: > > > > In file included from fsconfig01.c:9: > > > ../../../../include/lapi/fsmount.h:55:8: error: redefinition of > 'struct mount_attr' > > > 55 | struct mount_attr { > > > | ^~~~~~~~~~ > > > In file included from ../../../../include/lapi/fsmount.h:14: > > > > /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: > note: originally defined here > > > 210 | struct mount_attr > > > | ^~~~~~~~~~ > > > > Fixes: > > > - > http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 > > > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > > > --- > > > configure.ac | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > > index c2b0f48e7..a6d8ac826 100644 > > > --- a/configure.ac > > > +++ b/configure.ac > > > @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, > struct __kernel_old_timespec, struc > > > > AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) > > > AC_CHECK_TYPES([struct mount_attr],,,[ > > > -#ifdef HAVE_LINUX_MOUNT_H > > > -# include <linux/mount.h> > > > -#else > > > +#ifdef HAVE_MOUNT_SETATTR > > > # include <sys/mount.h> > > > +#elif HAVE_LINUX_MOUNT_H > > > +# include <linux/mount.h> > > > #endif > > > ]) > > > I wonder if we can get this whole mess of two different fallback headers > > simplified. Looking at the glibc implementation it seems to include > > "linux/mount.h" if it does exist. So most reasonable solution would do > > the same I guess which we did before the commit you reference. > > > @Li Wang where does the the sys/mount.h and linux/mount.h conflict? As > > far as I can tell the UAPI linux/mount.h never defined any functions, > > just constants, which should be solvable without the complex ifdefs, we > > just need to check for existence of the types and define them only if > > missing. > > Quoting [1]: > > The newer Glibc already provided wrapper for the series pidfd syscall, > so let's include the header file conditionally. > > # rpm -q glibc-devel > glibc-devel-2.35.9000-31.fc37.ppc64le > > # rpm -ql glibc-devel | grep pidfd > /usr/include/sys/pidfd.h > > To get rid of compiling error from fedora-rawhide: > > tst_safe_macros.c: In function ‘safe_pidfd_open’: > tst_safe_macros.c:135:16: error: implicit declaration of function > ‘pidfd_open’ [-Werror=implicit-function-declaration] > 135 | rval = pidfd_open(pid, flags); > | > > => Li, does the problem still persists? If yes, I'd be for merging this > patch. > Could you also test it on newest glibc in Fedora? > Of course yes, I have requested a fedora-rawhide for the LTP building, but tomorrow is the soonest I can provide the result :). > > Cyril, FYI musl never includes linux headers (Rich Felker has always been > against this practise). > > Kind regards, > Petr > > [1] > https://lore.kernel.org/ltp/20220805063401.1647479-2-liwang@redhat.com/ > >
On Mon, Feb 27, 2023 at 7:26 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with > > latest glibc which provides mount_attr in sys/mount.h. Unfortunately, > > the following build failure is still raised because sys/mount is now > > unconditionally included in include/lapi/fsmount.h: > > > > In file included from fsconfig01.c:9: > > ../../../../include/lapi/fsmount.h:55:8: error: redefinition of 'struct > mount_attr' > > 55 | struct mount_attr { > > | ^~~~~~~~~~ > > In file included from ../../../../include/lapi/fsmount.h:14: > > > /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: > note: originally defined here > > 210 | struct mount_attr > > | ^~~~~~~~~~ > > > > Fixes: > > - > http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 > > > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > > --- > > configure.ac | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > index c2b0f48e7..a6d8ac826 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, > struct __kernel_old_timespec, struc > > > > AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) > > AC_CHECK_TYPES([struct mount_attr],,,[ > > -#ifdef HAVE_LINUX_MOUNT_H > > -# include <linux/mount.h> > > -#else > > +#ifdef HAVE_MOUNT_SETATTR > > # include <sys/mount.h> > > +#elif HAVE_LINUX_MOUNT_H > > +# include <linux/mount.h> > > #endif > > ]) > > I wonder if we can get this whole mess of two different fallback headers > simplified. Looking at the glibc implementation it seems to include > "linux/mount.h" if it does exist. So most reasonable solution would do > the same I guess which we did before the commit you reference. > This is indeed correct if only face the latest Glibc, but that might have problems when building LTP on a middle version of Glibc/Kernel-headers. The bug I mentioned in the last email was fixed since glibc-2.37~426. @Fabrice, what kind of version of Glibc/Kernel-headers do you use? and which platform? Btw, this patch builds LTP successfully in CI: - https://github.com/wangli5665/ltp/actions/runs/4380739470 And I manually tried with fedora-rawhide/fedora34/35/37/38 all looks good. Maybe we can just apply this patch to keep everything with no big changes. @Cyril, Petr, what do you think?
Hi all, ... > > I wonder if we can get this whole mess of two different fallback headers > > simplified. Looking at the glibc implementation it seems to include > > "linux/mount.h" if it does exist. So most reasonable solution would do > > the same I guess which we did before the commit you reference. > This is indeed correct if only face the latest Glibc, but that might have > problems when building LTP on a middle version of Glibc/Kernel-headers. > The bug I mentioned in the last email was fixed since glibc-2.37~426. Do you know which exact commit has fixed it? It'd help us to realize when this fix is not needed any more (it'd be good to put it into the commit message). > @Fabrice, what kind of version of Glibc/Kernel-headers do you use? and > which platform? http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865/config arch: arm # Buildroot 2023.02-rc1-22-ga39e328bb2 Configuration => 2.36-81-g4f4d7a13edfd2fdc57c9d76e1fd6d017fb47550c kernel headers 5.10. Specific Buildroot toolchain. > Btw, this patch builds LTP successfully in CI: > - https://github.com/wangli5665/ltp/actions/runs/4380739470 Buildroot often founds problems for embedded folks, because they use different toolchain than traditional linux distros. > And I manually tried with fedora-rawhide/fedora34/35/37/38 all looks good. Thanks for checking. > Maybe we can just apply this patch to keep everything with no big changes. > @Cyril, Petr, what do you think? Unless there is a simple way to improve things, I'm for merging this.
On Fri, Mar 10, 2023 at 5:31 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi all, > > ... > > > I wonder if we can get this whole mess of two different fallback > headers > > > simplified. Looking at the glibc implementation it seems to include > > > "linux/mount.h" if it does exist. So most reasonable solution would do > > > the same I guess which we did before the commit you reference. > > > > This is indeed correct if only face the latest Glibc, but that might have > > problems when building LTP on a middle version of Glibc/Kernel-headers. > > The bug I mentioned in the last email was fixed since glibc-2.37~426. > Do you know which exact commit has fixed it? It'd help us to realize when > this > fix is not needed any more (it'd be good to put it into the commit > message). > I mean this commit: https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 > > > @Fabrice, what kind of version of Glibc/Kernel-headers do you use? and > > which platform? > > > http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865/config > arch: arm > # Buildroot 2023.02-rc1-22-ga39e328bb2 Configuration > => 2.36-81-g4f4d7a13edfd2fdc57c9d76e1fd6d017fb47550c > kernel headers 5.10. > Specific Buildroot toolchain. > > > Btw, this patch builds LTP successfully in CI: > > - https://github.com/wangli5665/ltp/actions/runs/4380739470 > Buildroot often founds problems for embedded folks, because they use > different > toolchain than traditional linux distros. > > > And I manually tried with fedora-rawhide/fedora34/35/37/38 all looks > good. > Thanks for checking. > > > Maybe we can just apply this patch to keep everything with no big > changes. > > > @Cyril, Petr, what do you think? > Unless there is a simple way to improve things, I'm for merging this. > >
Hi Li, > On Fri, Mar 10, 2023 at 5:31 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi all, > > ... > > > > I wonder if we can get this whole mess of two different fallback > > headers > > > > simplified. Looking at the glibc implementation it seems to include > > > > "linux/mount.h" if it does exist. So most reasonable solution would do > > > > the same I guess which we did before the commit you reference. > > > This is indeed correct if only face the latest Glibc, but that might have > > > problems when building LTP on a middle version of Glibc/Kernel-headers. > > > The bug I mentioned in the last email was fixed since glibc-2.37~426. > > Do you know which exact commit has fixed it? It'd help us to realize when > > this > > fix is not needed any more (it'd be good to put it into the commit > > message). > I mean this commit: > https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 Thanks for info. Fix from 2.37, it was also cherry picked to release/2.36/master branch, but not to branches for older releases (2.35 and 2.34). I suppose there will be always some toolchain/distro with older glibc without this fix, thus I'd merge it now, and made simplification Cyril suggested sometimes next year. Kind regards, Petr
Hi all, ... > > I mean this commit: > > https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 > Thanks for info. Fix from 2.37, it was also cherry picked to release/2.36/master > branch, but not to branches for older releases (2.35 and 2.34). > I suppose there will be always some toolchain/distro with older glibc without > this fix, thus I'd merge it now, and made simplification Cyril suggested > sometimes next year. I finally merged this patch adding info why original workaround from b857f8723 is needed. Li, thanks for your input, Fabrice, thanks for the fix and your patience. Kind regards, Petr > Kind regards, > Petr
diff --git a/configure.ac b/configure.ac index c2b0f48e7..a6d8ac826 100644 --- a/configure.ac +++ b/configure.ac @@ -225,10 +225,10 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struc AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) AC_CHECK_TYPES([struct mount_attr],,,[ -#ifdef HAVE_LINUX_MOUNT_H -# include <linux/mount.h> -#else +#ifdef HAVE_MOUNT_SETATTR # include <sys/mount.h> +#elif HAVE_LINUX_MOUNT_H +# include <linux/mount.h> #endif ])
Commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 tried to fix build with latest glibc which provides mount_attr in sys/mount.h. Unfortunately, the following build failure is still raised because sys/mount is now unconditionally included in include/lapi/fsmount.h: In file included from fsconfig01.c:9: ../../../../include/lapi/fsmount.h:55:8: error: redefinition of 'struct mount_attr' 55 | struct mount_attr { | ^~~~~~~~~~ In file included from ../../../../include/lapi/fsmount.h:14: /home/autobuild/autobuild/instance-4/output-1/host/armeb-buildroot-linux-gnueabi/sysroot/usr/include/sys/mount.h:210:8: note: originally defined here 210 | struct mount_attr | ^~~~~~~~~~ Fixes: - http://autobuild.buildroot.org/results/4dbb72e1bf081afd3cd944571b9beeefc7608865 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)