Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 9 Mar 2020 at 13:23, Liu, Jingqi <jingqi.liu@intel.com> wrote:
>
> On 3/6/2020 12:40 AM, Peter Maydell wrote:
> > On Thu, 5 Mar 2020 at 16:11, Ján Tomko <jtomko@redhat.com> wrote:
> >> On a Thursday in 2020, Jingqi Liu wrote:
> >>> The CONFIG_LINUX symbol is always not defined in this file.
> >>> This fixes that "config-host.h" header file is not included
> >>> for getting macros.
> >>>
> >>> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
> >>> ---
> >>> util/mmap-alloc.c | 2 ++
> >>> 1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> >>> index 27dcccd8ec..24c0e380f3 100644
> >>> --- a/util/mmap-alloc.c
> >>> +++ b/util/mmap-alloc.c
> >>> @@ -10,6 +10,8 @@
> >>>   * later.  See the COPYING file in the top-level directory.
> >>>   */
> >>>
> >>> +#include "config-host.h"
> >>> +
> >> According to CODING_STYLE.rst, qemu/osdep.h is the header file
> >> that should be included first, before all the other includes.
> >>
> >> So the minimal fix would be moving qemu/osdep.h up here.
> > Yes, osdep must always be first.
> >
> >>> #ifdef CONFIG_LINUX
> >>> #include <linux/mman.h>
> >>> #else  /* !CONFIG_LINUX */
> > Do we really need this? osdep.h will pull in sys/mman.h
> > for you, which should define the MAP_* constants.
> >
> > Also, you have no fallbmack for "I'm on Linux but the
> > system headers don't define MAP_SHARED_VALIDATE or
> > MAP_SYNC". Wouldn't it be better to just have
> > #ifndef MAP_SYNC
> > #define MAP_SYNC 0
> > #endif
> >
> > etc ?
> osdep.h pulls in sys/mman.h, which defines the MAP_* constants
>
> except for MAP_SYNC and MAP_SHARED_VALIDATE on Linux.

Why not? Is this just "not yet in the version of glibc
we're using", or is it a bug/missed feature in glibc
that needs to be addressed there ?

> How about just adding the following code in util/mmap-alloc.c ?

> #ifndef MAP_SYNC
> #define MAP_SYNC 0x80000
> #endif
>
> #ifndef MAP_SHARED_VALIDATE
> #define MAP_SHARED_VALIDATE 0x03
> #endif

You don't want to do that for non-Linux systems, so there
you need to fall back to defining them to be 0.

Are there any systems (distros) where the standard system
sys/mman.h does not define these new MAP_* constants but we
still really really need to use them? If not, then we
could just have the fallback-to-0 fallback everywhere.

thanks
-- PMM