Re: [PATCH 2/6] tools: Introduce a non-truncating xc_xenver_extraversion()

[Christian Lindig <christian.lindig@xxxxxxxxxx>]

> On 17 Jan 2023, at 13:53, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> ... which uses XENVER_extraversion2.
> 
> In order to do sensibly, use manual hypercall buffer handling.  Not only does
> this avoid an extra bounce buffer (we need to strip the xen_varbuf_t header
> anyway), it's also shorter and easlier to follow.
> 
> Update libxl and the ocaml stubs to match.  No API/ABI change in either.
> 
> With this change made, `xl info` can now correctly access a >15 char
> extraversion:
> 
>  # xl info xen_version
>  4.18-unstable+REALLY LONG EXTRAVERSION
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Wei Liu <wl@xxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
> CC: David Scott <dave@xxxxxxxxxx>
> CC: Edwin Torok <edvin.torok@xxxxxxxxxx>
> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>

Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>

> 
> Note: There is a marginal risk for a memory leak in the ocaml bindings, but
> it turns out we have the same bug elsewhere and fixing that is going to be
> rather complicated.
> ---
> tools/include/xenctrl.h             |  6 +++
> tools/libs/ctrl/xc_version.c        | 75 +++++++++++++++++++++++++++++++++++++
> tools/libs/light/libxl.c            |  4 +-
> tools/ocaml/libs/xc/xenctrl_stubs.c |  9 +++--
> 4 files changed, 87 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 23037874d3d5..1e88d49371a4 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1604,6 +1604,12 @@ long xc_memory_op(xc_interface *xch, unsigned int cmd, 
> void *arg, size_t len);
> 
> int xc_version(xc_interface *xch, int cmd, void *arg);
> 
> +/*
> + * Wrappers around XENVER_* subops.  Callers must pass the returned pointer 
> to
> + * free().
> + */
> +char *xc_xenver_extraversion(xc_interface *xch);
> +
> int xc_flask_op(xc_interface *xch, xen_flask_op_t *op);
> 
> /*
> diff --git a/tools/libs/ctrl/xc_version.c b/tools/libs/ctrl/xc_version.c
> index 60e107dcee0b..2c14474feec5 100644
> --- a/tools/libs/ctrl/xc_version.c
> +++ b/tools/libs/ctrl/xc_version.c
> @@ -81,3 +81,78 @@ int xc_version(xc_interface *xch, int cmd, void *arg)
> 
>     return rc;
> }
> +
> +/*
> + * Raw hypercall wrapper, letting us pass NULL and things which aren't of
> + * xc_hypercall_buffer_t *.
> + */
> +static int do_xen_version_raw(xc_interface *xch, int cmd, void *hbuf)
> +{
> +    return xencall2(xch->xcall, __HYPERVISOR_xen_version,
> +                    cmd, (unsigned long)hbuf);
> +}
> +
> +/*
> + * Issues a xen_varbuf_t subop, using manual hypercall buffer handling to
> + * avoid unnecessary buffering.
> + *
> + * On failure, returns NULL.  errno probably useful.
> + * On success, returns a pointer which must be freed with 
> xencall_free_buffer().
> + */
> +static xen_varbuf_t *varbuf_op(xc_interface *xch, unsigned int subop)
> +{
> +    xen_varbuf_t *hbuf = NULL;
> +    ssize_t sz;
> +
> +    sz = do_xen_version_raw(xch, subop, NULL);
> +    if ( sz < 0 )
> +        return NULL;
> +
> +    hbuf = xencall_alloc_buffer(xch->xcall, sizeof(*hbuf) + sz);
> +    if ( !hbuf )
> +        return NULL;
> +
> +    hbuf->len = sz;
> +
> +    sz = do_xen_version_raw(xch, subop, hbuf);
> +    if ( sz < 0 )
> +    {
> +        xencall_free_buffer(xch->xcall, hbuf);
> +        return NULL;
> +    }
> +
> +    hbuf->len = sz;
> +    return hbuf;
> +}
> +
> +/*
> + * Wrap varbuf_op() to obtain a simple string.  Copy out of the hypercall
> + * buffer, stripping the xen_varbuf_t header and appending a NUL terminator.
> + *
> + * On failure, returns NULL, errno probably useful.
> + * On success, returns a pointer which must be free()'d.
> + */
> +static char *varbuf_simple_string(xc_interface *xch, unsigned int subop)
> +{
> +    xen_varbuf_t *hbuf = varbuf_op(xch, subop);
> +    char *res;
> +
> +    if ( !hbuf )
> +        return NULL;
> +
> +    res = malloc(hbuf->len + 1);
> +    if ( res )
> +    {
> +        memcpy(res, hbuf->buf, hbuf->len);
> +        res[hbuf->len] = '\0';
> +    }
> +
> +    xencall_free_buffer(xch->xcall, hbuf);
> +
> +    return res;
> +}
> +
> +char *xc_xenver_extraversion(xc_interface *xch)
> +{
> +    return varbuf_simple_string(xch, XENVER_extraversion2);
> +}
> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
> index a0bf7d186f69..3e16e568839c 100644
> --- a/tools/libs/light/libxl.c
> +++ b/tools/libs/light/libxl.c
> @@ -581,7 +581,6 @@ const libxl_version_info* 
> libxl_get_version_info(libxl_ctx *ctx)
> {
>     GC_INIT(ctx);
>     union {
> -        xen_extraversion_t xen_extra;
>         xen_compile_info_t xen_cc;
>         xen_changeset_info_t xen_chgset;
>         xen_capabilities_info_t xen_caps;
> @@ -600,8 +599,7 @@ const libxl_version_info* 
> libxl_get_version_info(libxl_ctx *ctx)
>     info->xen_version_major = xen_version >> 16;
>     info->xen_version_minor = xen_version & 0xFF;
> 
> -    xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> -    info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
> +    info->xen_version_extra = xc_xenver_extraversion(ctx->xch);
> 
>     xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
>     info->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 2fba9c5e94d6..f3ce12dd8683 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -929,9 +929,8 @@ CAMLprim value stub_xc_version_version(value xch)
> {
>       CAMLparam1(xch);
>       CAMLlocal1(result);
> -     xen_extraversion_t extra;
> +     char *extra;
>       long packed;
> -     int retval;
> 
>       caml_enter_blocking_section();
>       packed = xc_version(_H(xch), XENVER_version, NULL);
> @@ -941,10 +940,10 @@ CAMLprim value stub_xc_version_version(value xch)
>               failwith_xc(_H(xch));
> 
>       caml_enter_blocking_section();
> -     retval = xc_version(_H(xch), XENVER_extraversion, &extra);
> +     extra = xc_xenver_extraversion(_H(xch));
>       caml_leave_blocking_section();
> 
> -     if (retval)
> +     if (!extra)
>               failwith_xc(_H(xch));
> 
>       result = caml_alloc_tuple(3);
> @@ -953,6 +952,8 @@ CAMLprim value stub_xc_version_version(value xch)
>       Store_field(result, 1, Val_int(packed & 0xffff));
>       Store_field(result, 2, caml_copy_string(extra));
> 
> +     free(extra);
> +
>       CAMLreturn(result);
> }
> 
> -- 
> 2.11.0
>