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

["Liu, Jingqi" <jingqi.liu@intel.com>]

On 3/9/2020 9:35 PM, Peter Maydell wrote:
> 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 ?

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
>> #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.

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