Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Florian Fainelli <f.fainelli@gmail.com>]

On 7/20/2022 4:28 PM, Darrick J. Wong wrote:
> Can one of you please apply this patch and see if it'll build in musl on
> mips, please?  Sorry it's taken so long to address this. :/
> 
> --D
> 
> ---
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Florian Fainelli most recently reported that xfsprogs doesn't build with
> musl on mips:
> 
> "MIPS platforms building with recent kernel headers and the musl-libc
> toolchain will expose the following build failure:
> 
> mmap.c: In function 'mmap_f':
> mmap.c:196:12: error: 'MAP_SYNC' undeclared (first use in this function); did you mean 'MS_SYNC'?
>    196 |    flags = MAP_SYNC | MAP_SHARED_VALIDATE;
>        |            ^~~~~~~~
>        |            MS_SYNC
> mmap.c:196:12: note: each undeclared identifier is reported only once for each function it appears in
> make[4]: *** [../include/buildrules:81: mmap.o] Error 1"
> 
> At first glance, the build failure here is caused by the fact that:
> 
> 1. The configure script doesn't detect MAP_SYNC support
> 2. The build system doesn't set HAVE_MAP_SYNC
> 2. io/mmap.c includes input.h -> projects.h -> xfs.h and later sys/mman.h
> 3. include/linux.h #define's MAP_SYNC to 0 if HAVE_MAP_SYNC is not set
> 4. musl's sys/mman.h #undef MAP_SYNC on platforms that don't support it
> 5. io/mmap.c tries to use MAP_SYNC, not realizing that libc undefined it
> 
> Normally, xfs_io only exports functionality that is defined by the libc
> and/or kernel headers on the build system.  We often make exceptions for
> new functionality so that we have a way to test them before the header
> file packages catch up, hence this '#ifndef HAVE_FOO #define FOO'
> paradigm.
> 
> MAP_SYNC is a gross and horribly broken example of this.  These support
> crutches are supposed to be *private* to xfsprogs for benefit of early
> testing, but they were instead added to include/linux.h, which we
> provide to user programs in the xfslibs-dev package.  IOWs, we've been
> #defining MAP_SYNC to zero for unsuspecting programs.
> 
> Worst yet, gcc 11.3 doesn't even warn about overriding a #define to 0:
> 
> #include <stdio.h>
> #include <sys/mman.h>
> #ifdef STUPID
> # include <xfs/xfs.h>
> #endif
> 
> int main(int argc, char *argv[]) {
> 	printf("MAP_SYNC 0x%x\n", MAP_SYNC);
> }
> 
> $ gcc -o a a.c -Wall
> $ ./a
> MAP_SYNC 0x80000
> $ gcc -DSTUPID -o a a.c -Wall
> $ ./a
> MAP_SYNC 0x0
> 
> Four years have gone by since the introduction of MAP_SYNC, so let's get
> rid of the override code entirely -- any platform that supports MAP_SYNC
> has had plenty of chances to ensure their header files have the right
> bits.  While we're at it, fix AC_HAVE_MAP_SYNC to look for MAP_SYNC in
> the same header file that the one user (io/mmap.c) uses -- sys/mman.h.
> 
> Annoyingly, I had to test this by hand because the sole fstest that
> exercises MAP_SYNC (generic/470) requires dm-logwrites and dm-thinp,
> neither of which support fsdax on current kernels.
> 
> Reported-by: info@mobile-stream.com
> Reported-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian