Re: [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative return values

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> Instead of assuming everything is always OK. We stash
> the gpfns value as an parameter.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_core_arm.c    | 17 ++++++++++++++---
>  tools/libxc/xc_core_x86.c    | 24 ++++++++++++++++++++----
>  tools/libxc/xc_domain_save.c |  8 +++++++-
>  3 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
> index 16508e7..26cec04 100644
> --- a/tools/libxc/xc_core_arm.c
> +++ b/tools/libxc/xc_core_arm.c
> @@ -31,9 +31,16 @@ xc_core_arch_gpfn_may_present(struct xc_core_arch_context *arch_ctxt,
>  }
>  
> 
> -static int nr_gpfns(xc_interface *xch, domid_t domid)
> +static int nr_gpfns(xc_interface *xch, domid_t domid, unsigned long *gpfns)

You didn't fancy merging the two versions of this then ;-)
> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index d8846f1..02377e8 100644
> --- a/tools/libxc/xc_core_x86.c
> +++ b/tools/libxc/xc_core_x86.c

> @@ -88,7 +99,12 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc
>      int err;
>      int i;
>  
> -    dinfo->p2m_size = nr_gpfns(xch, info->domid);
> +    err = nr_gpfns(xch, info->domid, &dinfo->p2m_size);

Please could you avoid reusing err here, the reason is that it's sole
use now is to save errno over the cleanup path, whereas here it looks
like it is going to be used for something but it isn't.

 if ( nr_gpfns(...)  < 0 )

is ok per the Xen coding style if you don't actually need the return
code.

Or

    ret = nr_gpfns()
    if ( ret < 0 )
        error, goto out

    ret = -1;
    .. the rest

would be ok too I guess. (coding style here allows
    if ( (ret = nr_gpfns(...)) < 0 )
too FWIW).

> +    if ( err < 0 )
> +    {
> +        ERROR("nr_gpfns returns errno: %d.", errno);
> +        goto out;
> +    }
>      if ( dinfo->p2m_size < info->nr_pages  )
>      {
>          ERROR("p2m_size < nr_pages -1 (%lx < %lx", dinfo->p2m_size, info->nr_pages - 1);
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 254fdb3..6346c12 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -939,7 +939,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>      }
>  
>      /* Get the size of the P2M table */
> -    dinfo->p2m_size = xc_domain_maximum_gpfn(xch, dom) + 1;
> +    rc = xc_domain_maximum_gpfn(xch, dom);
> +    if ( rc < 0 )
> +    {
> +        ERROR("Could not get maximum GPFN!");
> +        goto out;
> +    }
> +    dinfo->p2m_size = rc + 1;

Shame this can't use the same helper as the others.

Ian.