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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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

--
Daniel De Graaf
National Security Agency

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