> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Wednesday, October 12, 2016 3:41 PM
> To: Gonglei (Arei); Michael Tokarev; qemu-devel@nongnu.org
> Cc: qemu-triv...@nongnu.org; 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:m...@tls.msk.ru]
> >> 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 <arei.gong...@huawei.com>
> >>> ---
> >>> 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 */
> >>>
> >
> >
> >
