qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cao jin
Subject: Re: [Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using
Date: Wed, 26 Oct 2016 18:14:06 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0



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 <address@hidden>
---
  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




.







reply via email to

[Prev in Thread] Current Thread [Next in Thread]