Re: [Xen-devel] [PATCH 08/14] libxl: Fix xc_tmem_control to return proper error.

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> The API returns now negative values on error and stashes
> the error in errno. Fix the user of this API.
> 
> The 'xc_hypercall_bounce_pre' can fail - and if so it will
> stash its errno values - no need to over-write it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  tools/libxc/xc_tmem.c                  | 14 ++++++++++----
>  tools/xenstat/libxenstat/src/xenstat.c |  5 +++--
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> index 3261e10..02797bf 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -73,11 +73,14 @@ int xc_tmem_control(xc_interface *xch,
>      if ( subop == TMEMC_LIST && arg1 != 0 )
>      {
>          if ( buf == NULL )
> -            return -EINVAL;
> +        {
> +            errno = EINVAL;
> +            return -1;
> +        }
>          if ( xc_hypercall_bounce_pre(xch, buf) )
>          {
>              PERROR("Could not bounce buffer for tmem control hypercall");
> -            return -ENOMEM;
> +            return -1;
>          }
>      }
>  
> @@ -118,11 +121,14 @@ int xc_tmem_control_oid(xc_interface *xch,
>      if ( subop == TMEMC_LIST && arg1 != 0 )
>      {
>          if ( buf == NULL )
> -            return -EINVAL;
> +        {
> +            errno = EINVAL;
> +            return -1;
> +        }
>          if ( xc_hypercall_bounce_pre(xch, buf) )
>          {
>              PERROR("Could not bounce buffer for tmem control (OID) 
> hypercall");
> -            return -ENOMEM;
> +            return -1;
>          }
>      }
>  
> diff --git a/tools/xenstat/libxenstat/src/xenstat.c 
> b/tools/xenstat/libxenstat/src/xenstat.c
> index 8072a90..bf257ef 100644
> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -166,6 +166,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, 
> unsigned int flags)
>       xc_domaininfo_t domaininfo[DOMAIN_CHUNK_SIZE];
>       int new_domains;
>       unsigned int i;
> +     long rc;
>  
>       /* Create the node */
>       node = (xenstat_node *) calloc(1, sizeof(xenstat_node));
> @@ -189,9 +190,9 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, 
> unsigned int flags)
>       node->free_mem = ((unsigned long long)physinfo.free_pages)
>           * handle->page_size;
>  
> -     node->freeable_mb = (long)xc_tmem_control(handle->xc_handle, -1,
> +     rc = (long)xc_tmem_control(handle->xc_handle, -1,
>                               TMEMC_QUERY_FREEABLE_MB, -1, 0, 0, 0, NULL);

Why the cast, why not make rc an int since that is what xc_tmem_control
takes and you don't seem to use the full width anyway?

Or alternatively fix the return type of xc_tmem_control.

> -
> +     node->freeable_mb = (rc < 0) ? 0 : rc;

Should rc not get propagated into an error for the caller?

>       /* malloc(0) is not portable, so allocate a single domain.  This will
>        * be resized below. */
>       node->domains = malloc(sizeof(xenstat_domain));



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