Re: [PATCH XEN v3 14/22] tools: foreignmemory: provide xenforeignmemory_unmap.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/10/15 15:15, Ian Campbell wrote:

What is the plan WRT fixing the existing APIs?

The two options are 1) Move to new libs then fix, or 2) Fix in tree then
move to pristine new libs.

Option 1) runs the risk of digging ourself a hole in the form of a
stable API.  Option 2) seems like it would be clearer to review.

> diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
> index 01cd42e..86a5a97 100644
> --- a/tools/libs/foreignmemory/linux.c
> +++ b/tools/libs/foreignmemory/linux.c
> @@ -276,6 +276,12 @@ void *xenforeignmemory_map(xenforeignmemory_handle *fmem,
>      return addr;
>  }
>  
> +int xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
> +                           void *addr, unsigned int num)
> +{
> +    return munmap(addr, (unsigned long)num << PAGE_SHIFT);

This cast indicates that unsigned int is not appropriate in the API.

It matches the map() side, but severely risks truncating the unmap()
because of the internal multiplication by 4096.  Both the map and unmap
side should use "size_t num" rather than unsigned int.

> diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
> index 2e41af8..1a0d76a 100644
> --- a/tools/libxc/xc_sr_restore.c
> +++ b/tools/libxc/xc_sr_restore.c
> @@ -378,7 +378,7 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned count,
>  
>   err:
>      if ( mapping )
> -        munmap(mapping, nr_pages * PAGE_SIZE);
> +        xenforeignmemory_unmap(xch->fmem, mapping, nr_pages * PAGE_SIZE);

The semantics are different between munmap() and
xenforeignmemory_unmap(), which means you need to drop the * PAGE_SIZE 
in each of the replacements.

~Andrew