[PATCH] src/detached_mounts_propagation: Fix compile error

[PATCH] src/detached_mounts_propagation: Fix compile error

[Yang Xu]

Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
in addition to <linux/mount.h>(we also include this kernel header when using
global.h).

Usually we should use glibc header instead of kernel header.
But now mount.h is a special case because both new glibc header and
kernel header all define "struct mount_attr'. They also define MS*
macro.

Since we have some syscall wrapper in vfs/missing.h, we can use
<linux.mount.h> directly instead of <sys/mount.h>.

In fact, newer glibc(2.37-1)[1] has sloved conflict problem between
<sys/mount.h> and <linux/mount.h>. In the future(maybe ten years),
we can remove this kernel header and use glibc header.

[1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/detached_mounts_propagation.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
index 17db2c02..dd11f7be 100644
--- a/src/detached_mounts_propagation.c
+++ b/src/detached_mounts_propagation.c
@@ -20,7 +20,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
@@ -127,7 +126,7 @@ int main(int argc, char *argv[])
 	if (ret < 0)
 		exit_log("%m - Failed to create new mount namespace");
 
-	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
+	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
 	if (ret < 0)
 		exit_log("%m - Failed to make base_dir shared mountpoint");
 
@@ -174,7 +173,7 @@ int main(int argc, char *argv[])
 		}
 		close(fd_tree);
 
-		ret = umount2(target, MNT_DETACH);
+		ret = sys_umount2(target, MNT_DETACH);
 		if (ret < 0) {
 			fprintf(stderr, "%m - Failed to unmount %s", target);
 			exit_code = EXIT_FAILURE;
-- 
2.39.1

Re: [PATCH] src/detached_mounts_propagation: Fix compile error

[Christian Brauner]

On Tue, Apr 25, 2023 at 01:33:54PM +0800, Yang Xu wrote:
> Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
> in addition to <linux/mount.h>(we also include this kernel header when using
> global.h).
> 
> Usually we should use glibc header instead of kernel header.
> But now mount.h is a special case because both new glibc header and
> kernel header all define "struct mount_attr'. They also define MS*
> macro.
> 
> Since we have some syscall wrapper in vfs/missing.h, we can use
> <linux.mount.h> directly instead of <sys/mount.h>.
> 
> In fact, newer glibc(2.37-1)[1] has sloved conflict problem between
> <sys/mount.h> and <linux/mount.h>. In the future(maybe ten years),
> we can remove this kernel header and use glibc header.
> 
> [1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

There are neither enough drugs nor curses to express how enraging this
header mess is. I've given up trying to understand what the "correct"
solution is. So if this thing below works,

Acked-by: Christian Brauner <brauner@kernel.org>

>  src/detached_mounts_propagation.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
> index 17db2c02..dd11f7be 100644
> --- a/src/detached_mounts_propagation.c
> +++ b/src/detached_mounts_propagation.c
> @@ -20,7 +20,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <sys/mount.h>
>  #include <sys/stat.h>
>  #include <sys/syscall.h>
>  #include <sys/types.h>
> @@ -127,7 +126,7 @@ int main(int argc, char *argv[])
>  	if (ret < 0)
>  		exit_log("%m - Failed to create new mount namespace");
>  
> -	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
> +	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
>  	if (ret < 0)
>  		exit_log("%m - Failed to make base_dir shared mountpoint");
>  
> @@ -174,7 +173,7 @@ int main(int argc, char *argv[])
>  		}
>  		close(fd_tree);
>  
> -		ret = umount2(target, MNT_DETACH);
> +		ret = sys_umount2(target, MNT_DETACH);
>  		if (ret < 0) {
>  			fprintf(stderr, "%m - Failed to unmount %s", target);
>  			exit_code = EXIT_FAILURE;
> -- 
> 2.39.1
> 

Re: [PATCH] src/detached_mounts_propagation: Fix compile error

[Ziyang Zhang]

On 2023/4/25 13:33, Yang Xu wrote:
> Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
> in addition to <linux/mount.h>(we also include this kernel header when using
> global.h).
> 
> Usually we should use glibc header instead of kernel header.
> But now mount.h is a special case because both new glibc header and
> kernel header all define "struct mount_attr'. They also define MS*
> macro.
> 
> Since we have some syscall wrapper in vfs/missing.h, we can use
> <linux.mount.h> directly instead of <sys/mount.h>.
> 
> In fact, newer glibc(2.37-1)[1] has sloved conflict problem between
> <sys/mount.h> and <linux/mount.h>. In the future(maybe ten years),
> we can remove this kernel header and use glibc header.
> 
> [1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  src/detached_mounts_propagation.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
> index 17db2c02..dd11f7be 100644
> --- a/src/detached_mounts_propagation.c
> +++ b/src/detached_mounts_propagation.c
> @@ -20,7 +20,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <sys/mount.h>
>  #include <sys/stat.h>
>  #include <sys/syscall.h>
>  #include <sys/types.h>
> @@ -127,7 +126,7 @@ int main(int argc, char *argv[])
>  	if (ret < 0)
>  		exit_log("%m - Failed to create new mount namespace");
>  
> -	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
> +	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
>  	if (ret < 0)
>  		exit_log("%m - Failed to make base_dir shared mountpoint");
>  
> @@ -174,7 +173,7 @@ int main(int argc, char *argv[])
>  		}
>  		close(fd_tree);
>  
> -		ret = umount2(target, MNT_DETACH);
> +		ret = sys_umount2(target, MNT_DETACH);
>  		if (ret < 0) {
>  			fprintf(stderr, "%m - Failed to unmount %s", target);
>  			exit_code = EXIT_FAILURE;

Hi Yang,

Unfortunately, we find another error after applying this patch:

Building vfs
    [CC]    vfstest
In file included from utils.h:32,
                 from utils.c:21:
missing.h:115:8: error: redefinition of 'struct mount_attr'
  115 | struct mount_attr {
      |        ^~~~~~~~~~
In file included from utils.c:13:
/usr/include/sys/mount.h:210:8: note: originally defined here
  210 | struct mount_attr
      |        ^~~~~~~~~~
gmake[4]: *** [Makefile:30: vfstest] Error 1
gmake[3]: *** [../include/buildrules:31: vfs] Error 2
gmake[2]: *** [include/buildrules:31: src] Error 2
make[1]: *** [Makefile:51: default] Error 2
make: *** [Makefile:49: default] Error 2


Looks like we have to fix src/vfs/utils.c too because it includes both <sys/mount.h>
and "utils.h" and "utils.h" includes "missing.h".

I simply remove #include <sys/mount.h> in src/vfs/utils.c and it works:

diff --git a/src/vfs/utils.c b/src/vfs/utils.c
index 6db7a11d..f30e3bd9 100644
--- a/src/vfs/utils.c
+++ b/src/vfs/utils.c
@@ -10,7 +10,6 @@
 #include <stdlib.h>
 #include <sys/eventfd.h>
 #include <sys/fsuid.h>
-#include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/socket.h>
 #include <sys/stat.h>

Re: [PATCH] src/detached_mounts_propagation: Fix compile error

[Yang Xu (Fujitsu)]

on 2023/04/25 15:47, Ziyang Zhang wrote:
> On 2023/4/25 13:33, Yang Xu wrote:
>> Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
>> in addition to <linux/mount.h>(we also include this kernel header when using
>> global.h).
>>
>> Usually we should use glibc header instead of kernel header.
>> But now mount.h is a special case because both new glibc header and
>> kernel header all define "struct mount_attr'. They also define MS*
>> macro.
>>
>> Since we have some syscall wrapper in vfs/missing.h, we can use
>> <linux.mount.h> directly instead of <sys/mount.h>.
>>
>> In fact, newer glibc(2.37-1)[1] has sloved conflict problem between
>> <sys/mount.h> and <linux/mount.h>. In the future(maybe ten years),
>> we can remove this kernel header and use glibc header.
>>
>> [1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>   src/detached_mounts_propagation.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
>> index 17db2c02..dd11f7be 100644
>> --- a/src/detached_mounts_propagation.c
>> +++ b/src/detached_mounts_propagation.c
>> @@ -20,7 +20,6 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>> -#include <sys/mount.h>
>>   #include <sys/stat.h>
>>   #include <sys/syscall.h>
>>   #include <sys/types.h>
>> @@ -127,7 +126,7 @@ int main(int argc, char *argv[])
>>   	if (ret < 0)
>>   		exit_log("%m - Failed to create new mount namespace");
>>   
>> -	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
>> +	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
>>   	if (ret < 0)
>>   		exit_log("%m - Failed to make base_dir shared mountpoint");
>>   
>> @@ -174,7 +173,7 @@ int main(int argc, char *argv[])
>>   		}
>>   		close(fd_tree);
>>   
>> -		ret = umount2(target, MNT_DETACH);
>> +		ret = sys_umount2(target, MNT_DETACH);
>>   		if (ret < 0) {
>>   			fprintf(stderr, "%m - Failed to unmount %s", target);
>>   			exit_code = EXIT_FAILURE;
> 
> Hi Yang,
> 
> Unfortunately, we find another error after applying this patch:
> 
> Building vfs
>      [CC]    vfstest
> In file included from utils.h:32,
>                   from utils.c:21:
> missing.h:115:8: error: redefinition of 'struct mount_attr'
>    115 | struct mount_attr {
>        |        ^~~~~~~~~~
> In file included from utils.c:13:
> /usr/include/sys/mount.h:210:8: note: originally defined here
>    210 | struct mount_attr
>        |        ^~~~~~~~~~
> gmake[4]: *** [Makefile:30: vfstest] Error 1
> gmake[3]: *** [../include/buildrules:31: vfs] Error 2
> gmake[2]: *** [include/buildrules:31: src] Error 2
> make[1]: *** [Makefile:51: default] Error 2
> make: *** [Makefile:49: default] Error 2
> 
> 

I have searched the placse of include <sys/mount.h>

as below:

m4/package_libcdev.m4:88:#include <sys/mount.h>
src/aio-dio-regress/aiodio_sparse2.c:23:#include <sys/mount.h>
src/ext4_resize.c:15:#include <sys/mount.h>
src/getdevicesize.c:17:#include <sys/mount.h>
src/vfs/utils.c:13:#include <sys/mount.h>
autom4te.cache/traces.0:1929:#include <sys/mount.h>
autom4te.cache/output.0:13858:# include <sys/mount.h>
autom4te.cache/output.1:13858:# include <sys/mount.h

only utils.c include xfstests owner header, other c source files should 
not be affected. I will remove  <sys/mount.h> in utils.c and then send a v2.

Best Regards
Yang Xu
> Looks like we have to fix src/vfs/utils.c too because it includes both <sys/mount.h>
> and "utils.h" and "utils.h" includes "missing.h".
> 
> I simply remove #include <sys/mount.h> in src/vfs/utils.c and it works:
> 
> diff --git a/src/vfs/utils.c b/src/vfs/utils.c
> index 6db7a11d..f30e3bd9 100644
> --- a/src/vfs/utils.c
> +++ b/src/vfs/utils.c
> @@ -10,7 +10,6 @@
>   #include <stdlib.h>
>   #include <sys/eventfd.h>
>   #include <sys/fsuid.h>
> -#include <sys/mount.h>
>   #include <sys/prctl.h>
>   #include <sys/socket.h>
>   #include <sys/stat.h>

[PATCH v2] src: Don't include <sys/mount.h> and <linux/mount.h> together

[Yang Xu]

Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
in addition to <linux/mount.h>.

Usually we should use glibc header instead of kernel header.
But now mount.h is a special case because both new glibc header and
kernel header all define "struct mount_attr'. They also define MS* macro.

Since we have some syscall wrapper in vfs/missing.h, we can use <linux.mount.h> directly
instead of <sys/mount.h> for detached_mounts_propagation.c.

For utils.c, it doesn't use the macro or function in <sys/mount.h>, so
remove it directly.

In fact, newer glibc(2.37-1)[1] has sloved conflict problem between <sys/mount.h> and <linux/mount.h>.
In the future(maybe ten years), we can remove this kernel header and use glibc header.

[1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6

Acked-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/detached_mounts_propagation.c | 5 ++---
 src/vfs/utils.c                   | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
index 17db2c02..dd11f7be 100644
--- a/src/detached_mounts_propagation.c
+++ b/src/detached_mounts_propagation.c
@@ -20,7 +20,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
@@ -127,7 +126,7 @@ int main(int argc, char *argv[])
 	if (ret < 0)
 		exit_log("%m - Failed to create new mount namespace");
 
-	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
+	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
 	if (ret < 0)
 		exit_log("%m - Failed to make base_dir shared mountpoint");
 
@@ -174,7 +173,7 @@ int main(int argc, char *argv[])
 		}
 		close(fd_tree);
 
-		ret = umount2(target, MNT_DETACH);
+		ret = sys_umount2(target, MNT_DETACH);
 		if (ret < 0) {
 			fprintf(stderr, "%m - Failed to unmount %s", target);
 			exit_code = EXIT_FAILURE;
diff --git a/src/vfs/utils.c b/src/vfs/utils.c
index 9e67ac37..0ab5de15 100644
--- a/src/vfs/utils.c
+++ b/src/vfs/utils.c
@@ -10,7 +10,6 @@
 #include <stdlib.h>
 #include <sys/eventfd.h>
 #include <sys/fsuid.h>
-#include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
-- 
2.39.1

Re: [PATCH v2] src: Don't include <sys/mount.h> and <linux/mount.h> together

[Ziyang Zhang]

On 2023/4/25 16:21, Yang Xu wrote:
> Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
> in addition to <linux/mount.h>.
> 
> Usually we should use glibc header instead of kernel header.
> But now mount.h is a special case because both new glibc header and
> kernel header all define "struct mount_attr'. They also define MS* macro.
> 
> Since we have some syscall wrapper in vfs/missing.h, we can use <linux.mount.h> directly
> instead of <sys/mount.h> for detached_mounts_propagation.c.
> 
> For utils.c, it doesn't use the macro or function in <sys/mount.h>, so
> remove it directly.
> 
> In fact, newer glibc(2.37-1)[1] has sloved conflict problem between <sys/mount.h> and <linux/mount.h>.
> In the future(maybe ten years), we can remove this kernel header and use glibc header.
> 
> [1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6
> 
> Acked-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Tested-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH v2] src: Don't include <sys/mount.h> and <linux/mount.h> together

[Zorro Lang]

On Tue, Apr 25, 2023 at 04:21:30PM +0800, Yang Xu wrote:
> Newer glibc such as glibc 2.36 also defines 'struct mount_attr'
> in addition to <linux/mount.h>.
> 
> Usually we should use glibc header instead of kernel header.
> But now mount.h is a special case because both new glibc header and
> kernel header all define "struct mount_attr'. They also define MS* macro.
> 
> Since we have some syscall wrapper in vfs/missing.h, we can use <linux.mount.h> directly
> instead of <sys/mount.h> for detached_mounts_propagation.c.
> 
> For utils.c, it doesn't use the macro or function in <sys/mount.h>, so
> remove it directly.
> 
> In fact, newer glibc(2.37-1)[1] has sloved conflict problem between <sys/mount.h> and <linux/mount.h>.
> In the future(maybe ten years), we can remove this kernel header and use glibc header.
> 
> [1]https://sourceware.org/git/?p=glibc.git;a=commit;h=774058d72942249f71d74e7f2b639f77184160a6
> 
> Acked-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

The commit log makes sense to me. (Except some lines are too long, I prefer
each general lines in commit log less than 70 chars. I'll adjust that a bit
hen I merge it).

This version looks good to me, and doesn't break RHEL 8/9 system this time.
Thanks!

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  src/detached_mounts_propagation.c | 5 ++---
>  src/vfs/utils.c                   | 1 -
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/detached_mounts_propagation.c b/src/detached_mounts_propagation.c
> index 17db2c02..dd11f7be 100644
> --- a/src/detached_mounts_propagation.c
> +++ b/src/detached_mounts_propagation.c
> @@ -20,7 +20,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <sys/mount.h>
>  #include <sys/stat.h>
>  #include <sys/syscall.h>
>  #include <sys/types.h>
> @@ -127,7 +126,7 @@ int main(int argc, char *argv[])
>  	if (ret < 0)
>  		exit_log("%m - Failed to create new mount namespace");
>  
> -	ret = mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
> +	ret = sys_mount(NULL, base_dir, NULL, MS_REC | MS_SHARED, NULL);
>  	if (ret < 0)
>  		exit_log("%m - Failed to make base_dir shared mountpoint");
>  
> @@ -174,7 +173,7 @@ int main(int argc, char *argv[])
>  		}
>  		close(fd_tree);
>  
> -		ret = umount2(target, MNT_DETACH);
> +		ret = sys_umount2(target, MNT_DETACH);
>  		if (ret < 0) {
>  			fprintf(stderr, "%m - Failed to unmount %s", target);
>  			exit_code = EXIT_FAILURE;
> diff --git a/src/vfs/utils.c b/src/vfs/utils.c
> index 9e67ac37..0ab5de15 100644
> --- a/src/vfs/utils.c
> +++ b/src/vfs/utils.c
> @@ -10,7 +10,6 @@
>  #include <stdlib.h>
>  #include <sys/eventfd.h>
>  #include <sys/fsuid.h>
> -#include <sys/mount.h>
>  #include <sys/prctl.h>
>  #include <sys/socket.h>
>  #include <sys/stat.h>
> -- 
> 2.39.1
>