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....@intel.com> wrote: > > > On 3/6/2020 12:40 AM, Peter Maydell wrote: > > > > On Thu, 5 Mar 2020 at 16:11, Ján Tomko <jto...@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....@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
