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

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

[Jingqi Liu]

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"
+
 #ifdef CONFIG_LINUX
 #include <linux/mman.h>
 #else  /* !CONFIG_LINUX */
-- 
2.17.1

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

[Ján Tomko]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

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.

> #ifdef CONFIG_LINUX
> #include <linux/mman.h>
> #else  /* !CONFIG_LINUX */


Introduced by commit 119906afa5ca610adb87c55ab0d8e53c9104bfc3

Jano

>-- 
>2.17.1
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

thanks
-- PMM

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

[Liu, Jingqi]

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 ?

Thanks for your review.

The system headers bits/mman.h (which is included by sys/mman.h)

don't define MAP_SYNC and MAP_SHARED_VALIDATE on linux.

They're defined in linux-headers/asm-generic/mman-common.h ( which is 
included by linux/mman.h).

Another MAP_* macros are defined in both header files.

As you mentioned, if don't include linux/mman.h, just defines as following:

#ifndef MAP_SYNC
#define MAP_SYNC 0
#endif

The MAP_SYNC is always 0 since MAP_SYNC isn't defined in system header.

Thanks,

Jingqi

>
> thanks
> -- PMM

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

[Liu, Jingqi]

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.

How about just adding the following code in util/mmap-alloc.c ?

#ifndef MAP_SYNC
#define MAP_SYNC 0x80000
#endif
#ifndef MAP_SYNC
#define MAP_SYNC 0x80000
#endif

#ifndef MAP_SHARED_VALIDATE
#define MAP_SHARED_VALIDATE 0x03
#endif

Thanks,
Jingqi

>
> thanks
> -- PMM

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

[Peter Maydell]

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 ?

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

thanks
-- PMM

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

[Liu, Jingqi]

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

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

[Michael S. Tsirkin]

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.

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

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

[Liu, Jingqi]

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

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

[Peter Maydell]

On Wed, 11 Mar 2020 at 00:43, Liu, Jingqi <jingqi.liu@intel.com> wrote:
> 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.

osdep.h first, always. Other uses of linux-headers headers
have presumably already dealt with this issue...

thanks
-- PMM

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

[Eduardo Habkost]

On Wed, Mar 11, 2020 at 12:37:17PM +0000, Peter Maydell wrote:
> On Wed, 11 Mar 2020 at 00:43, Liu, Jingqi <jingqi.liu@intel.com> wrote:
> > 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.
> 
> osdep.h first, always. Other uses of linux-headers headers
> have presumably already dealt with this issue...

Including linux/mman.h before sys/mman.h is just a workaround to
the root cause: both headers really redefine each others' macros,
but gcc hide the warnings if the warnings are generated inside
system-provided headers.

I believe we should use -isystem for linux-headers insteaad of -I.

-- 
Eduardo

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

[Liu, Jingqi]

On 3/6/2020 12:10 AM, Ján Tomko 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.
>
Thanks for your review.

I've tried to simply move "qemu/osdep.h" to the first line.

It caused many macros redefinition errors.


>> #ifdef CONFIG_LINUX
>> #include <linux/mman.h>
>> #else  /* !CONFIG_LINUX */
>
>
> Introduced by commit 119906afa5ca610adb87c55ab0d8e53c9104bfc3

I've checked this commit.

Thanks,

Jingqi

>
> Jano
>
>> -- 
>> 2.17.1
>>
>>