On 7/21/22 05:11, Carlos Maiolino wrote:
> On Wed, Jul 20, 2022 at 04:28:00PM -0700, 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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxx
>> Reported-by: Fabrice Fontaine <fontaine.fabrice@xxxxxxxxx>
>> Reported-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>> ---
>> include/linux.h | 8 --------
>> io/io.h | 2 +-
>> io/mmap.c | 25 +++++++++++++------------
>> m4/package_libcdev.m4 | 3 +--
>> 4 files changed, 15 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/linux.h b/include/linux.h
>> index 3d9f4e3d..eddc4ad9 100644
>> --- a/include/linux.h
>> +++ b/include/linux.h
>> @@ -251,14 +251,6 @@ struct fsxattr {
>> #define FS_XFLAG_COWEXTSIZE 0x00010000 /* CoW extent size allocator hint */
>> #endif
>>
>> -#ifndef HAVE_MAP_SYNC
>> -#define MAP_SYNC 0
>> -#define MAP_SHARED_VALIDATE 0
>> -#else
>> -#include <asm-generic/mman.h>
>> -#include <asm-generic/mman-common.h>
>> -#endif /* HAVE_MAP_SYNC */
>> -
>> /*
>> * Reminder: anything added to this file will be compiled into downstream
>> * userspace projects!
>> diff --git a/io/io.h b/io/io.h
>> index ada0a149..de4ef607 100644
>> --- a/io/io.h
>> +++ b/io/io.h
>> @@ -58,7 +58,7 @@ typedef struct mmap_region {
>> size_t length; /* length of mapping */
>> off64_t offset; /* start offset into backing file */
>> int prot; /* protection mode of the mapping */
>> - bool map_sync; /* is this a MAP_SYNC mapping? */
>> + int flags; /* MAP_* flags passed to mmap() */
>> char *name; /* name of backing file */
>> } mmap_region_t;
>>
>> diff --git a/io/mmap.c b/io/mmap.c
>> index 8c048a0a..425957d4 100644
>> --- a/io/mmap.c
>> +++ b/io/mmap.c
>> @@ -46,8 +46,11 @@ print_mapping(
>> for (i = 0, p = pflags; p->prot != PROT_NONE; i++, p++)
>> buffer[i] = (map->prot & p->prot) ? p->mode : '-';
>>
>> - if (map->map_sync)
>> +#ifdef HAVE_MAP_SYNC
>> + if ((map->flags & (MAP_SYNC | MAP_SHARED_VALIDATE)) ==
>> + (MAP_SYNC | MAP_SHARED_VALIDATE))
>> sprintf(&buffer[i], " S");
>> +#endif
>>
>> printf("%c%03d%c 0x%lx - 0x%lx %s %14s (%lld : %ld)\n",
>> braces? '[' : ' ', index, braces? ']' : ' ',
>> @@ -139,7 +142,9 @@ mmap_help(void)
>> " -r -- map with PROT_READ protection\n"
>> " -w -- map with PROT_WRITE protection\n"
>> " -x -- map with PROT_EXEC protection\n"
>> +#ifdef HAVE_MAP_SYNC
>> " -S -- map with MAP_SYNC and MAP_SHARED_VALIDATE flags\n"
>> +#endif
>> " -s <size> -- first do mmap(size)/munmap(size), try to reserve some free space\n"
>> " If no protection mode is specified, all are used by default.\n"
>> "\n"));
>> @@ -193,18 +198,14 @@ mmap_f(
>> prot |= PROT_EXEC;
>> break;
>> case 'S':
>> +#ifdef HAVE_MAP_SYNC
>> flags = MAP_SYNC | MAP_SHARED_VALIDATE;
>> -
>> - /*
>> - * If MAP_SYNC and MAP_SHARED_VALIDATE aren't defined
>> - * in the system headers we will have defined them
>> - * both as 0.
>> - */
>> - if (!flags) {
>> - printf("MAP_SYNC not supported\n");
>> - return 0;
>> - }
>> break;
>> +#else
>> + printf("MAP_SYNC not supported\n");
>> + exitcode = 1;
>> + return command_usage(&mmap_cmd);
>> +#endif
>> case 's':
>> length2 = cvtnum(blocksize, sectsize, optarg);
>> break;
>> @@ -281,7 +282,7 @@ mmap_f(
>> mapping->offset = offset;
>> mapping->name = filename;
>> mapping->prot = prot;
>> - mapping->map_sync = (flags == (MAP_SYNC | MAP_SHARED_VALIDATE));
>> + mapping->flags = flags;
>> return 0;
>> }
>>
>> diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
>> index df44174d..5293dd1a 100644
>> --- a/m4/package_libcdev.m4
>> +++ b/m4/package_libcdev.m4
>> @@ -387,8 +387,7 @@ AC_DEFUN([AC_HAVE_MAP_SYNC],
>> [ AC_MSG_CHECKING([for MAP_SYNC])
>> AC_COMPILE_IFELSE(
>> [ AC_LANG_PROGRAM([[
>> -#include <asm-generic/mman.h>
>> -#include <asm-generic/mman-common.h>
>> +#include <sys/mman.h>
>> ]], [[
>> int flags = MAP_SYNC | MAP_SHARED_VALIDATE;
>> ]])
>
> Patch looks good, if you plan to make it into a non-RFC patch, feel free to
> include:
>
> Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
>
Darrick, do you need to re-post, or can the maintainers pick up the patch directly?
--
Florian
