Re: [PATCH v4 05/34] libxc/libxl/python/xenstat: Use new XEN_VERSION_OP hypercall

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

On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote:
> We change the xen_version libxc code to use the new hypercall.
> Which of course means every user in the code base has to
> be changed over.
>
> It is important to note that the xc_version_op has a different
> return semantic than the previous one. It returns negative
> values on error (like the old one), but it also returns
> an positive value on success (unlike the old one). The positive
> value is the number of bytes copied in.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: David Scott <dave@recoil.org>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>
> v2: Use xc_version_op_val_t instead of uint32 or such
> v3: Make sure to check ret < 0 instead of ret (as it returns the size) -
>     in Ocaml code. Found by Andrew.
> v4: Update comment for xc_version to mention the return the size
> ---
>  tools/libxc/include/xenctrl.h          | 24 ++++++++++-
>  tools/libxc/xc_core.c                  | 35 +++++++--------
>  tools/libxc/xc_dom_boot.c              | 12 +++++-
>  tools/libxc/xc_domain.c                |  3 +-
>  tools/libxc/xc_private.c               | 53 ++++-------------------
>  tools/libxc/xc_private.h               |  7 +--
>  tools/libxc/xc_resume.c                |  3 +-
>  tools/libxc/xc_sr_save.c               |  9 ++--
>  tools/libxc/xg_save_restore.h          |  6 ++-
>  tools/libxl/libxl.c                    | 79 ++++++++++++++++++++++------------
>  tools/ocaml/libs/xc/xenctrl_stubs.c    | 39 +++++++----------
>  tools/python/xen/lowlevel/xc/xc.c      | 30 +++++++------
>  tools/xenstat/libxenstat/src/xenstat.c | 12 +++---
>  tools/xentrace/xenctx.c                |  3 +-
>  14 files changed, 169 insertions(+), 146 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 150d727..379de30 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1477,7 +1477,29 @@ int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>  int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
>  int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>  
> -int xc_version(xc_interface *xch, int cmd, void *arg);
> +/**
> + * This function returns the size of buffer to be allocated for
> + * the cmd. The cmd are XEN_VERSION_OP_*.
> + */

This should return ssize_t as it could also plausibly return -1 and set
errno.

> +int xc_version_len(xc_interface *xch, unsigned int cmd);
> +/**
> + * This function retrieves the information from the version_op hypercall.
> + * The len is the size of the arg buffer. If arg is NULL, will not
> + * perform hypercall - instead will just return the size of arg
> + * buffer that is needed.
> + *
> + * Note that prior to Xen 4.7 this would return 0 for success and
> + * negative value (-1) for error (with the error in errno). In Xen 4.7
> + * and later for success it will return an positive value which is the
> + * number of bytes copied in arg.
> + *
> + * It can also return -1 with various errno values:
> + *  - EPERM - not permitted.
> + *  - ENOBUFS - the len was to short, output in arg truncated.
> + *  - ENOSYS - not implemented.
> + *
> + */
> +int xc_version(xc_interface *xch, unsigned int cmd, void *arg, ssize_t len);

This can get away with taking a size_t len.  I am not sure how much we
care about people trying to claim negative length for *arg.

> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index c40a4e9..23876f0 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1204,34 +1204,40 @@ static PyObject *pyxc_xeninfo(XcObject *self)
>      xen_capabilities_info_t xen_caps;
>      xen_platform_parameters_t p_parms;
>      xen_commandline_t xen_commandline;
> -    long xen_version;
> -    long xen_pagesize;
> +    xen_version_op_val_t xen_version;
> +    xen_version_op_val_t xen_pagesize;
>      char str[128];
>  
> -    xen_version = xc_version(self->xc_handle, XENVER_version, NULL);
> -
> -    if ( xc_version(self->xc_handle, XENVER_extraversion, &xen_extra) != 0 )
> +    if ( xc_version(self->xc_handle, XEN_VERSION_OP_version, &xen_version,
> +                    sizeof(xen_version)) < 0)

Xen Style.

> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -621,20 +621,18 @@ unsigned long long xenstat_network_tdrop(xenstat_network * network)
>  /* Collect Xen version information */
>  static int xenstat_collect_xen_version(xenstat_node * node)
>  {
> -	long vnum = 0;
> +	xen_version_op_val_t vnum = 0;
>  	xen_extraversion_t version;
>  
>  	/* Collect Xen version information if not already collected */
>  	if (node->handle->xen_version[0] == '\0') {
>  		/* Get the Xen version number and extraversion string */
> -		vnum = xc_version(node->handle->xc_handle,
> -			XENVER_version, NULL);
> -
> -		if (vnum < 0)
> +		if (xc_version(node->handle->xc_handle,
> +			           XEN_VERSION_OP_version, &vnum, sizeof(vnum)) < 0 )
>  			return 0;

Curiously, the opposite style bug here.

With these trivial bits fixed, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>, and a Tested-by: for the Ocaml stubs
(eventually).

~Andrew

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