util/mmap-alloc: check parameter before using

Also refactor some code hunk for readability

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 util/mmap-alloc.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

On 26.10.2016 09:18, Cao jin wrote:
> Also refactor some code hunk for readability
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  util/mmap-alloc.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 5a85aa3..92c123a 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -41,6 +41,11 @@ size_t qemu_fd_getpagesize(int fd)
>  
>  void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>  {
> +    /* Make sure align is a power of 2 */
> +    assert(!(align & (align - 1)));
> +    /* Always align to host page size */
> +    assert(align >= getpagesize());

Now you've moved code before the declaration of the local variables.
That's also some kind of ugly (and might not work with older versions of
C compilers?)...

>      /*
>       * Note: this always allocates at least one extra page of virtual address
>       * space, even if size is already aligned.
> @@ -68,11 +73,6 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>          return MAP_FAILED;
>      }
>  
> -    /* Make sure align is a power of 2 */
> -    assert(!(align & (align - 1)));
> -    /* Always align to host page size */
> -    assert(align >= getpagesize());

... so maybe simply move the setting of "offset" (which requires
"align") here right in front of the mmap() instead?

>      ptr1 = mmap(ptr + offset, size, PROT_READ | PROT_WRITE,
>                  MAP_FIXED |
>                  (fd == -1 ? MAP_ANONYMOUS : 0) |
> @@ -83,22 +83,20 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>          return MAP_FAILED;
>      }
>  
> -    ptr += offset;
> -    total -= offset;
> -
>      if (offset > 0) {
> -        munmap(ptr - offset, offset);
> +        munmap(ptr, offset);
>      }
>  
>      /*
>       * Leave a single PROT_NONE page allocated after the RAM block, to serve as
>       * a guard page guarding against potential buffer overflows.
>       */
> +    total -= offset;
>      if (total > size + getpagesize()) {
> -        munmap(ptr + size + getpagesize(), total - size - getpagesize());
> +        munmap(ptr1 + size + getpagesize(), total - size - getpagesize());
>      }
>  
> -    return ptr;
> +    return ptr1;
>  }

 Thomas

On 10/26/2016 04:10 PM, Thomas Huth wrote:
> On 26.10.2016 09:18, Cao jin wrote:
>> Also refactor some code hunk for readability
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   util/mmap-alloc.c | 20 +++++++++-----------
>>   1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>> index 5a85aa3..92c123a 100644
>> --- a/util/mmap-alloc.c
>> +++ b/util/mmap-alloc.c
>> @@ -41,6 +41,11 @@ size_t qemu_fd_getpagesize(int fd)
>>
>>   void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>>   {
>> +    /* Make sure align is a power of 2 */
>> +    assert(!(align & (align - 1)));
>> +    /* Always align to host page size */
>> +    assert(align >= getpagesize());
>
> Now you've moved code before the declaration of the local variables.
> That's also some kind of ugly (and might not work with older versions of
> C compilers?)...
>

Yes, I think it is ugly too, my personal taste is declaration should 
exist at the head of the function body, then parameters' check, then 
assignment to the variables in separate lines, but for this patch, I 
think that won't be liked by maintainers:)

>>       /*
>>        * Note: this always allocates at least one extra page of virtual address
>>        * space, even if size is already aligned.
>> @@ -68,11 +73,6 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>>           return MAP_FAILED;
>>       }
>>
>> -    /* Make sure align is a power of 2 */
>> -    assert(!(align & (align - 1)));
>> -    /* Always align to host page size */
>> -    assert(align >= getpagesize());
>
> ... so maybe simply move the setting of "offset" (which requires
> "align") here right in front of the mmap() instead?

A better choice to me:) Thanks

Cao jin

>
>>       ptr1 = mmap(ptr + offset, size, PROT_READ | PROT_WRITE,
>>                   MAP_FIXED |
>>                   (fd == -1 ? MAP_ANONYMOUS : 0) |
>> @@ -83,22 +83,20 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>>           return MAP_FAILED;
>>       }
>>
>> -    ptr += offset;
>> -    total -= offset;
>> -
>>       if (offset > 0) {
>> -        munmap(ptr - offset, offset);
>> +        munmap(ptr, offset);
>>       }
>>
>>       /*
>>        * Leave a single PROT_NONE page allocated after the RAM block, to serve as
>>        * a guard page guarding against potential buffer overflows.
>>        */
>> +    total -= offset;
>>       if (total > size + getpagesize()) {
>> -        munmap(ptr + size + getpagesize(), total - size - getpagesize());
>> +        munmap(ptr1 + size + getpagesize(), total - size - getpagesize());
>>       }
>>
>> -    return ptr;
>> +    return ptr1;
>>   }
>
>   Thomas
>
>
>
>
> .
>

On 10/26/2016 02:18 AM, Cao jin wrote:
> Also refactor some code hunk for readability
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  util/mmap-alloc.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 5a85aa3..92c123a 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -41,6 +41,11 @@ size_t qemu_fd_getpagesize(int fd)
>  
>  void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>  {
> +    /* Make sure align is a power of 2 */
> +    assert(!(align & (align - 1)));

Better to spell this:

assert(is_power_of_2(align));

at which point the comment is redundant and can be omitted.