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

[Cao jin <caoj.fnst@cn.fujitsu.com>]

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.
>
>   Thomas
>

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