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

[Peter Maydell]

On Thu, 5 Mar 2020 at 16:11, Ján Tomko <address@hidden> 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 <address@hidden>
> >---
> > 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 ?

thanks
-- PMM