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

[Cao jin]

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