|
From: | Liu, Jingqi |
Subject: | Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c |
Date: | Tue, 10 Mar 2020 16:58:38 +0800 |
User-agent: | Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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.#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 ?
I'm using the version 2.27 of glibc.I downloaded the version 2.28 of glibc source for compilation and installation.
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 #endif #ifndef MAP_SHARED_VALIDATE #define MAP_SHARED_VALIDATE 0x03 #endifYou 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 #endif #ifndef MAP_SHARED_VALIDATE #define MAP_SHARED_VALIDATE 0 #endif Thanks, Jingqi
thanks -- PMM
[Prev in Thread] | Current Thread | [Next in Thread] |