Re: [Qemu-trivial] [PATCH] mmap-alloc: check before use for ptr pointer

[Gonglei (Arei)]

> -----Original Message-----
> From: Paolo Bonzini [mailto:address@hidden On Behalf Of Paolo
> Bonzini
> Sent: Wednesday, October 12, 2016 3:41 PM
> To: Gonglei (Arei); Michael Tokarev; address@hidden
> Cc: address@hidden; Herongguang (Stephen)
> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
> 
> 
> 
> On 12/10/2016 09:37, Gonglei (Arei) wrote:
> >> -----Original Message-----
> >> From: Michael Tokarev [mailto:address@hidden
> >> Sent: Wednesday, October 12, 2016 2:46 PM
> >> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
> >>
> >> 12.10.2016 05:05, Gonglei wrote:
> >>> If ptr mmap failed, we don't need to do a superfluous
> >>> calculation for offset variable by ptr (MAP_FAILED).
> >>
> >> What's the point?  There's no problem in extra calculation
> >> if mmap failed, yes, but do we really care?  As of it is now,
> >> it is more compact and readable, and works.  I think.
> >>
> >
> > I just think it's more reasonable because the ptr is checked after
> > used. Why do we need a extra calculation?
> 
> Another reasonable objection (that however your patch doesn't fix) is
> that align is being used before the assertion:
> 
>      assert(!(align & (align - 1)));
> 

Eh, align is used at the beginning of the qemu_ram_mmap()  ;)


Regards,
-Gonglei

> Paolo
> 
> >
> > Regards,
> > -Gonglei
> >
> >> Thanks,
> >>
> >> /mjt
> >>
> >>> Signed-off-by: Gonglei <address@hidden>
> >>> ---
> >>>  util/mmap-alloc.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> >>> index 5a85aa3..577862b 100644
> >>> --- a/util/mmap-alloc.c
> >>> +++ b/util/mmap-alloc.c
> >>> @@ -61,13 +61,15 @@ void *qemu_ram_mmap(int fd, size_t size, size_t
> >> align, bool shared)
> >>>  #else
> >>>      void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS |
> >> MAP_PRIVATE, -1, 0);
> >>>  #endif
> >>> -    size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - 
> >>> (uintptr_t)ptr;
> >>> +    size_t offset;
> >>>      void *ptr1;
> >>>
> >>>      if (ptr == MAP_FAILED) {
> >>>          return MAP_FAILED;
> >>>      }
> >>>
> >>> +    offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
> >>> +
> >>>      /* Make sure align is a power of 2 */
> >>>      assert(!(align & (align - 1)));
> >>>      /* Always align to host page size */
> >>>
> >
> >
> >