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))); 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 */ >>> > > >