From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56464) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzJI1-00084v-Ct for qemu-devel@nongnu.org; Wed, 26 Oct 2016 04:10:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzJHw-0000sP-RN for qemu-devel@nongnu.org; Wed, 26 Oct 2016 04:10:25 -0400 References: <1477466334-4078-1-git-send-email-caoj.fnst@cn.fujitsu.com> From: Thomas Huth Message-ID: Date: Wed, 26 Oct 2016 10:10:16 +0200 MIME-Version: 1.0 In-Reply-To: <1477466334-4078-1-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] util/mmap-alloc: check parameter before using List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org, qemu-trivial@nongnu.org Cc: peter.maydell@linaro.org, gkurz@linux.vnet.ibm.com, armbru@redhat.com, mst@redhat.com On 26.10.2016 09:18, Cao jin wrote: > Also refactor some code hunk for readability > > Signed-off-by: Cao jin > --- > 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