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

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

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 <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
> 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 ?

Thanks,

Jingqi

>> Thanks,
>>
>> Jingqi
>>
>>> thanks
>>> -- PMM