[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Liu, Jingqi
Subject: Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
Date: Wed, 11 Mar 2020 08:43:18 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 3/10/2020 5:12 PM, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 04:58:38PM +0800, Liu, Jingqi wrote:
On 3/9/2020 9:35 PM, Peter Maydell wrote:
On Mon, 9 Mar 2020 at 13:23, Liu, Jingqi <address@hidden> wrote:
On 3/6/2020 12:40 AM, Peter Maydell wrote:
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.

#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

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 ?
I'm using the version 2.27 of glibc.

I downloaded the version 2.28 of glibc source for compilation and

I found MAP_SYNC and MAP_SHARED_VALIDATE are defined in this version.

Seems it's older glibc version issue.

How about just adding the following code in util/mmap-alloc.c ?
#ifndef MAP_SYNC
#define MAP_SYNC 0x80000

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.
Good point.

So as you mentioned, it would be better to just have the following code:

#ifndef MAP_SYNC
#define MAP_SYNC 0

Won't this defeat the purpose of MAP_SHARED_VALIDATE?

We really have linux-headers/linux/mman.h for exactly this purpose.

Yes, linux-headers/linux/mman.h has defined MAP_SYNC and MAP_SHARED_VALIDATE.

1) If '#include <linux/mman.h>' first then '#include qemu/osdep.h', it should be fine.

2) Peter mentioned osdep.h should go first.

It will  cause redefinitions of other MAP_* macros after '#include <linux/mman.h>'.

This is where the conflict lies.

Any comments ?





-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]