[Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using

[Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using

[Cao jin]

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());
+
     /*
      * 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());
-
     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;
 }
 
 void qemu_ram_munmap(void *ptr, size_t size)
-- 
2.1.0

Re: [Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using

[Cao jin]

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
>
>
>
>
> .
>

Re: [Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]