[Top][All Lists]

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

Re: [Qemu-trivial] [PATCH v3] util/mmap-alloc: check parameter before us

From: Cao jin
Subject: Re: [Qemu-trivial] [PATCH v3] util/mmap-alloc: check parameter before using
Date: Mon, 31 Oct 2016 19:17:49 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

On 10/31/2016 03:32 PM, Thomas Huth wrote:
On 31.10.2016 04:57, Cao jin wrote:

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.

I originally had similar problems as Michael understanding your changes
to ptr / ptr1 ... so you should likely at add the information with
MAP_FIXED etc. to the patch description at least, I think.

It's maybe also cleaner if you split your patch into two parts, first
patch to fix the parameter checking, and second patch to change the ptr1
stuff. That way the later patch could also be easier reverted in case
there are problems with that.


Oh, I didn't realize it would confuse people so easy, seems it actually did:(

Readability is kind of personal taste thing, generally, I woundn't send a patch only care readability which maybe controversial, so I am not sure this one worth the trouble to split the patch, but if everyone want it, I am pleased to do it. I was hoping someone(maybe author) could give a ack, or point out the error I made.

Will try to document it in next version.
Yours Sincerely,

Cao jin

reply via email to

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