* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-02-27 11:28 ` Cyril Hrubis
@ 2023-03-09 10:56 ` Petr Vorel
2023-03-09 13:09 ` Li Wang
2023-03-09 13:00 ` Li Wang
2023-03-10 6:25 ` Li Wang
2 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2023-03-09 10:56 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: Fabrice Fontaine, ltp
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/
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-03-09 10:56 ` Petr Vorel
@ 2023-03-09 13:09 ` Li Wang
0 siblings, 0 replies; 10+ messages in thread
From: Li Wang @ 2023-03-09 13:09 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp, Fabrice Fontaine
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/
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-02-27 11:28 ` Cyril Hrubis
2023-03-09 10:56 ` Petr Vorel
@ 2023-03-09 13:00 ` Li Wang
2023-03-10 6:25 ` Li Wang
2 siblings, 0 replies; 10+ messages in thread
From: Li Wang @ 2023-03-09 13:00 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: Fabrice Fontaine, ltp
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
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-02-27 11:28 ` Cyril Hrubis
2023-03-09 10:56 ` Petr Vorel
2023-03-09 13:00 ` Li Wang
@ 2023-03-10 6:25 ` Li Wang
2023-03-10 9:31 ` Petr Vorel
2 siblings, 1 reply; 10+ messages in thread
From: Li Wang @ 2023-03-10 6:25 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: Fabrice Fontaine, ltp
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?
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-03-10 6:25 ` Li Wang
@ 2023-03-10 9:31 ` Petr Vorel
2023-03-10 9:42 ` Li Wang
0 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2023-03-10 9:31 UTC (permalink / raw)
To: Li Wang; +Cc: ltp, Fabrice Fontaine
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.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-03-10 9:31 ` Petr Vorel
@ 2023-03-10 9:42 ` Li Wang
2023-03-10 11:42 ` Petr Vorel
0 siblings, 1 reply; 10+ messages in thread
From: Li Wang @ 2023-03-10 9:42 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp, Fabrice Fontaine
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.
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-03-10 9:42 ` Li Wang
@ 2023-03-10 11:42 ` Petr Vorel
2023-03-20 14:59 ` Petr Vorel
0 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2023-03-10 11:42 UTC (permalink / raw)
To: Li Wang; +Cc: ltp, Fabrice Fontaine
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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH] configure.ac: fix mount_attr detection
2023-03-10 11:42 ` Petr Vorel
@ 2023-03-20 14:59 ` Petr Vorel
0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2023-03-20 14:59 UTC (permalink / raw)
To: Li Wang, ltp, Fabrice Fontaine
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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread