Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Building master is broken by 2a068a1394d179dda4becf350e3afb4e8819675e #14057

Closed
ryao opened this issue Oct 19, 2022 · 7 comments · Fixed by #14058
Closed

Building master is broken by 2a068a1394d179dda4becf350e3afb4e8819675e #14057

ryao opened this issue Oct 19, 2022 · 7 comments · Fixed by #14058
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@ryao
Copy link
Contributor

ryao commented Oct 19, 2022

System information

Type Version/Name
Distribution Name Gentoo
Distribution Version 2.9
Kernel Version 5.16.14-gentoo-x86_64
Architecture x86_64
OpenZFS Version master

Describe the problem you're observing

Building master fails with:

tests/zfs-tests/cmd/idmap_util.c:103:8: error: redefinition of ‘struct mount_attr’
  103 | struct mount_attr {
      |        ^~~~~~~~~~
In file included from /usr/include/sys/mount.h:32,
                 from ./lib/libspl/include/os/linux/sys/mount.h:27,
                 from ./lib/libspl/include/os/linux/sys/stat.h:31,
                 from tests/zfs-tests/cmd/idmap_util.c:33:
/usr/include/linux/mount.h:129:8: note: originally defined here
  129 | struct mount_attr {
      |        ^~~~~~~~~~

Describe how to reproduce the problem

git clone https://github.com/openzfs/zfs.git
cd zfs
git clean -xdf && ./autogen.sh && ./configure --enable-debug --enable-debuginfo --with-config=all && make -j$(nproc)

2a068a1 introduced this code.

The problem code is:

#ifndef mount_attr
struct mount_attr {
        __u64 attr_set;
        __u64 attr_clr;
        __u64 propagation;
        __u64 userns_fd;
};
#endif

I am not sure how this was expected to work in the first place. You cannot use the CPP to check what C structures are defined. An autotools check is needed for that.
@ryao ryao added the Type: Defect Incorrect behavior (e.g. crash, hang) label Oct 19, 2022
ryao added a commit to ryao/zfs that referenced this issue Oct 19, 2022
@ryao
Fix multiple definitions of struct mount_attr on recent glibc versions
c3d3e91 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.

Closes openzfs#14057

Signed-off-by: Richard Yao <[email protected]>
@ryao ryao mentioned this issue Oct 19, 2022
Merged
@youzhongyang
Copy link
Contributor

I am also confused, how could it pass all the builds, including my manual ones on newer and older kernels. Anyway your fix looks good.

@ryao
Copy link
Contributor Author

ryao commented Oct 20, 2022

Older glibc versions that do not have a structure definition, so no problem would occur on them. The problem only occurs when the headers have the structure defined.

@youzhongyang
Copy link
Contributor

I am not convinced. Here are output from 3 different systems:

# grep 'struct mount_attr' /usr/include/linux/mount.h ; uname -r
#define MOUNT_ATTR_IDMAP        0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */
struct mount_attr {
5.15.0-46-lowlatency

# grep 'struct mount_attr' /usr/include/linux/mount.h ; uname -r
#define MOUNT_ATTR_IDMAP        0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */
struct mount_attr {
5.19.0-17-generic

# grep 'struct mount_attr' /usr/include/linux/mount.h ; uname -r
4.18.0-408.el8.x86_64

I have no problem building on them.

@ryao
Copy link
Contributor Author

ryao commented Oct 20, 2022

Do any of them use glibc 2.36?

@65a
Copy link

65a commented Oct 20, 2022

broken here as well, also on an affected glibc

@youzhongyang
Copy link
Contributor

ok I see why now. On Ubuntu if I install libc6-dev 2.36, its /usr/include/x86_64-linux-gnu/sys/mount.h has these new lines which would bring in the definition of struct mount_attr:

#ifdef __has_include
# if __has_include ("linux/mount.h")
#  include "linux/mount.h"
# endif
#endif

@ryao
Copy link
Contributor Author

ryao commented Oct 20, 2022

That explains things nicely. Thanks for finding it.

behlendorf pushed a commit that referenced this issue Oct 20, 2022
@ryao
Fix multiple definitions of struct mount_attr on recent glibc versions
9650b35 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #14057
Closes #14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Oct 21, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
783ffd7 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Oct 21, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
eea8d09 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Oct 21, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
096662c 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Oct 21, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
df0faba 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Nov 9, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
3847c5a 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Nov 9, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
9416398 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Nov 9, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
c8884e4 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
andrewc12 pushed a commit to andrewc12/openzfs that referenced this issue Nov 9, 2022
@ryao @andrewc12
Fix multiple definitions of struct mount_attr on recent glibc versions
3b12a16 
The ifdef used would never work because the CPP is not aware of C
structure definitions. Rather than use an autotools check, we can just
use a nameless structure that we typedef to mount_attr_t. This is a
Linux kernel interface, which means that it is stable and this is fine
to do.
    
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Youzhong Yang <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14057
Closes openzfs#14058
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix multiple definitions of struct mount_attr on recent glibc versions ryao/zfs
3 participants
@ryao @65a @youzhongyang