Re: [PATCH v2 2/2] mmap-alloc: Include osdep.h before checking CONFIG_LINUX

[Michael S. Tsirkin]

On Sun, Mar 15, 2020 at 11:45:59AM -0400, Eduardo Habkost wrote:
> On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote:
> > The CONFIG_LINUX check at the top of mmap-alloc.c never worked
> > because it was done before including osdep.h.
> > 
> > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set
> > to 0 at the beginning of the file.  Luckily, this didn't break
> > when using recent glibc versions (2.28+), because those macros
> > were redefined by glibc headers.
> > 
> > Move the CONFIG_LINUX check after the main include lines, so the
> > CONFIG_LINUX check works and we actually include <linux/mman.h>.
> > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if
> > the host has an older glibc version.

Wait a second, MAP_SHARED_VALIDATE is from
linux-headers/linux/mman.h - it's available on all architectures.

> > 
> > Reported-by: Jingqi Liu <address@hidden>
> > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> > * (none)
> > ---
> >  util/mmap-alloc.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> > index 27dcccd8ec..7c2ce98eb0 100644
> > --- a/util/mmap-alloc.c
> > +++ b/util/mmap-alloc.c
> > @@ -9,6 +9,9 @@
> >   * This work is licensed under the terms of the GNU GPL, version 2 or
> >   * later.  See the COPYING file in the top-level directory.
> >   */
> > +#include "qemu/osdep.h"
> > +#include "qemu/mmap-alloc.h"
> > +#include "qemu/host-utils.h"
> >  
> >  #ifdef CONFIG_LINUX
> >  #include <linux/mman.h>
> 
> This breaks the build on mips, because mips doesn't have MAP_SYNC
> defined at linux/mman.h:
> 
> https://app.shippable.com/github/ehabkost/qemu-hacks/runs/9/9/console


Oops. But that in fact means it's currently building on mips but not
working correctly there! MAP_SHARED_VALIDATE 0x0 is especially
problematic. I'm unsure what's the right thing to do is,
I guess as a first step we can go back and device MAP_SYNC to 0,






> 
> > @@ -17,10 +20,6 @@
> >  #define MAP_SHARED_VALIDATE   0x0
> >  #endif /* CONFIG_LINUX */
> >  
> > -#include "qemu/osdep.h"
> > -#include "qemu/mmap-alloc.h"
> > -#include "qemu/host-utils.h"
> > -
> >  #define HUGETLBFS_MAGIC       0x958458f6
> >  
> >  #ifdef CONFIG_LINUX
> > -- 
> > 2.24.1
> > 
> 
> -- 
> Eduardo