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

[Paolo Bonzini]

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

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