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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Mar 18, 2015 at 04:26:37PM +0000, Ian Campbell wrote:
> 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@oracle.com>
> > ---
> >  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?

Right. int should be enough.
> 
> 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?

Nope. If tmem is not enabled (so xc_tmem_control returns -ENOSYS)
freeable_mb should be zero. In this case we would have returned negative
values which is certainly not right.

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