Re: [Xen-devel] [PATCH] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-04-30 at 15:19 +0100, Daniel De Graaf wrote:
> On 04/30/2013 10:00 AM, Ian Campbell wrote:
> > On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote:
> >> On 04/30/2013 06:39 AM, Ian Campbell wrote:
> >>> On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
> >>>> On 04/26/2013 11:26 AM, Ian Campbell wrote:
> >>>>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
> >>>>>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
> >>>>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> >>>>>>>> "count" parameter should be pages count (as stated in comment in
> >>>>>>>> xenctrl.h), not bytes count.
> >>>>>>>> This patch fixes also the only user of this function (in xen 
> >>>>>>>> sources) -
> >>>>>>>> libvchan.
> >>>>>>>
> >>>>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
> >>>>>>
> >>>>>> This also looks good to me.
> >>>>>
> >>>>> May I take that as an Ack (or a Reviewed-by if you prefer)?
> >>>>
> >>>> Yes, either one is fine.
> >>>>
> >>>> Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >>>
> >>> Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
> >>> the changelog description (tut tut), correct? It seems like these
> >>> mappings can either be establish with xc_gntshr_share_pages or with "=
> >>> ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
> >>> case I'm concerned about... Should it not duplicate the switch used at
> >>> mapping time?
> >>>
> >>> Ian.
> >>>
> >>
> >> The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
> >> functional duplicate of the switch statement.
> >
> > It's a pretty opaque transformation ;-)
> >
> > But for e.g. order 9, it isn't the same is it? The switch on alloc will
> > hit the default case while the free won't hit this if statement.
> 
> Order 9 isn't valid: the only permitted values are 10, 11, and 12+, all of
> which are handled correctly here.

Ah, I missed the limit check just above, good.

> >>   However, this does bring
> >> up a different issue: the munmap() should be replaced with the correct
> >> one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
> >> in the same way as ctrl->ring is unmapped.
> >
> > OK, I take it the Ack is rescinded for the time being?
> >
> 
> The change is still a strict improvement over the old code, but since more
> changes are needed to complete the fix, it is rescinded for now.

It might be best to get the mechanical API change as a separate patch
(e.g. without adding new callers of the function, correct or otherwise)
and handle the correction of the munmap calls separately.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel