|
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 .
[Prev in Thread] | Current Thread | [Next in Thread] |