From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54335) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c13gm-00059N-Vt for qemu-devel@nongnu.org; Sun, 30 Oct 2016 23:55:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c13gj-00069e-0B for qemu-devel@nongnu.org; Sun, 30 Oct 2016 23:55:12 -0400 References: <1477644996-23990-1-git-send-email-caoj.fnst@cn.fujitsu.com> <92415a45-66a7-c080-2c3c-8f5f67204bd0@msgid.tls.msk.ru> From: Cao jin Message-ID: <5816C121.4060407@cn.fujitsu.com> Date: Mon, 31 Oct 2016 11:57:21 +0800 MIME-Version: 1.0 In-Reply-To: <92415a45-66a7-c080-2c3c-8f5f67204bd0@msgid.tls.msk.ru> Content-Type: text/plain; charset="windows-1251"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev , qemu-devel@nongnu.org, qemu-trivial@nongnu.org Cc: peter.maydell@linaro.org, thuth@redhat.com, eblake@redhat.com, armbru@redhat.com, mst@redhat.com On 10/28/2016 10:22 PM, Michael Tokarev wrote: > 28.10.2016 11:56, Cao jin wrote: >> Also refactor a little bit for readability > > See comments below... > >> Signed-off-by: Cao jin >> --- >> util/mmap-alloc.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c >> index 5a85aa3..2f55f5e 100644 >> --- a/util/mmap-alloc.c >> +++ b/util/mmap-alloc.c >> @@ -12,6 +12,7 @@ >> >> #include "qemu/osdep.h" >> #include "qemu/mmap-alloc.h" >> +#include "qemu/host-utils.h" >> >> #define HUGETLBFS_MAGIC 0x958458f6 >> >> @@ -61,18 +62,18 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared) >> #else >> void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); >> #endif >> - size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr; >> + size_t offset; >> void *ptr1; >> >> if (ptr == MAP_FAILED) { >> return MAP_FAILED; >> } >> >> - /* Make sure align is a power of 2 */ >> - assert(!(align & (align - 1))); >> + assert(is_power_of_2(align)); >> /* Always align to host page size */ >> assert(align >= getpagesize()); >> >> + offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr; >> ptr1 = mmap(ptr + offset, size, PROT_READ | PROT_WRITE, >> MAP_FIXED | >> (fd == -1 ? MAP_ANONYMOUS : 0) | >> @@ -83,22 +84,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; > > Why did you change ptr to ptr1 here and above? Because, I think there always is: ptr + offset == ptr1 > > On linux, mmap(2) manpage says that address serves as hint, and the > system create the mapping at a nearby page boundary. Generally, this > address is just a hint. So I'm not really sure if this code is actually > right.. :) > Yes, but the 2nd mmap used MAP_FIXED, which the manpage says: /Don't interpret addr as a hint: place the mapping at exactly that/ /address. addr must be a multiple of the page size/ /If the specified address cannot be used, mmap() will fail/ > At the very least, your commit comment is a bit misleading, as it says > about readability, but it also MAY change semantics. > I don't think so, one just need dig a little deeper:) > Maybe just move BOTH "ptr+=, total-=" parts down the line and keep > using ptr instead of ptr1? > > It'd be very good, in my opinion, to document how this whole thing > is supposed to work :) > the change is just some simple arithmetic operation, I think it is little difficult for me to find a decent description. Hope this could also got other maintainer's help, review, or ack > Thanks, > > /mjt > > > . > -- Yours Sincerely, Cao jin