Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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@tycho.nsa.gov>
>
> 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