* [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
@ 2020-03-05 15:41 Jingqi Liu
2020-03-05 16:10 ` Ján Tomko
0 siblings, 1 reply; 12+ messages in thread
From: Jingqi Liu @ 2020-03-05 15:41 UTC (permalink / raw)
To: mst, ehabkost; +Cc: Jingqi Liu, qemu-devel
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-05 15:41 [PATCH] util: fix to get configuration macros in util/mmap-alloc.c Jingqi Liu
@ 2020-03-05 16:10 ` Ján Tomko
2020-03-05 16:40 ` Peter Maydell
2020-03-06 2:48 ` Liu, Jingqi
0 siblings, 2 replies; 12+ messages in thread
From: Ján Tomko @ 2020-03-05 16:10 UTC (permalink / raw)
To: Jingqi Liu; +Cc: qemu-devel, ehabkost, mst
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-05 16:10 ` Ján Tomko
@ 2020-03-05 16:40 ` Peter Maydell
2020-03-06 4:01 ` Liu, Jingqi
2020-03-09 13:23 ` Liu, Jingqi
2020-03-06 2:48 ` Liu, Jingqi
1 sibling, 2 replies; 12+ messages in thread
From: Peter Maydell @ 2020-03-05 16:40 UTC (permalink / raw)
To: Ján Tomko
Cc: Jingqi Liu, Michael S. Tsirkin, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-05 16:10 ` Ján Tomko
2020-03-05 16:40 ` Peter Maydell
@ 2020-03-06 2:48 ` Liu, Jingqi
1 sibling, 0 replies; 12+ messages in thread
From: Liu, Jingqi @ 2020-03-06 2:48 UTC (permalink / raw)
To: Ján Tomko; +Cc: qemu-devel, ehabkost, mst
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
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-05 16:40 ` Peter Maydell
@ 2020-03-06 4:01 ` Liu, Jingqi
2020-03-09 13:23 ` Liu, Jingqi
1 sibling, 0 replies; 12+ messages in thread
From: Liu, Jingqi @ 2020-03-06 4:01 UTC (permalink / raw)
To: Peter Maydell, Ján Tomko
Cc: Michael S. Tsirkin, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-05 16:40 ` Peter Maydell
2020-03-06 4:01 ` Liu, Jingqi
@ 2020-03-09 13:23 ` Liu, Jingqi
2020-03-09 13:35 ` Peter Maydell
1 sibling, 1 reply; 12+ messages in thread
From: Liu, Jingqi @ 2020-03-09 13:23 UTC (permalink / raw)
To: Peter Maydell, Ján Tomko
Cc: Michael S. Tsirkin, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-09 13:23 ` Liu, Jingqi
@ 2020-03-09 13:35 ` Peter Maydell
2020-03-10 8:58 ` Liu, Jingqi
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2020-03-09 13:35 UTC (permalink / raw)
To: Liu, Jingqi
Cc: Michael S. Tsirkin, Ján Tomko, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-09 13:35 ` Peter Maydell
@ 2020-03-10 8:58 ` Liu, Jingqi
2020-03-10 9:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Liu, Jingqi @ 2020-03-10 8:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Michael S. Tsirkin, Ján Tomko, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-10 8:58 ` Liu, Jingqi
@ 2020-03-10 9:12 ` Michael S. Tsirkin
2020-03-11 0:43 ` Liu, Jingqi
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2020-03-10 9:12 UTC (permalink / raw)
To: Liu, Jingqi
Cc: Peter Maydell, Ján Tomko, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-10 9:12 ` Michael S. Tsirkin
@ 2020-03-11 0:43 ` Liu, Jingqi
2020-03-11 12:37 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Liu, Jingqi @ 2020-03-11 0:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Ján Tomko, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-11 0:43 ` Liu, Jingqi
@ 2020-03-11 12:37 ` Peter Maydell
2020-03-11 20:42 ` Eduardo Habkost
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2020-03-11 12:37 UTC (permalink / raw)
To: Liu, Jingqi
Cc: Ján Tomko, QEMU Developers, Eduardo Habkost, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c
2020-03-11 12:37 ` Peter Maydell
@ 2020-03-11 20:42 ` Eduardo Habkost
0 siblings, 0 replies; 12+ messages in thread
From: Eduardo Habkost @ 2020-03-11 20:42 UTC (permalink / raw)
To: Peter Maydell
Cc: Liu, Jingqi, Ján Tomko, QEMU Developers, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-03-11 20:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-05 15:41 [PATCH] util: fix to get configuration macros in util/mmap-alloc.c Jingqi Liu
2020-03-05 16:10 ` Ján Tomko
2020-03-05 16:40 ` Peter Maydell
2020-03-06 4:01 ` Liu, Jingqi
2020-03-09 13:23 ` Liu, Jingqi
2020-03-09 13:35 ` Peter Maydell
2020-03-10 8:58 ` Liu, Jingqi
2020-03-10 9:12 ` Michael S. Tsirkin
2020-03-11 0:43 ` Liu, Jingqi
2020-03-11 12:37 ` Peter Maydell
2020-03-11 20:42 ` Eduardo Habkost
2020-03-06 2:48 ` Liu, Jingqi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.