> On 17 Jan 2023, at 13:53, Andrew Cooper <andrew.coop...@citrix.com> 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.coop...@citrix.com>
> ---
> CC: Wei Liu <w...@xen.org>
> CC: Anthony PERARD <anthony.per...@citrix.com>
> CC: Juergen Gross <jgr...@suse.com>
> CC: Christian Lindig <christian.lin...@citrix.com>
> CC: David Scott <d...@recoil.org>
> CC: Edwin Torok <edvin.to...@citrix.com>
> CC: Rob Hoes <rob.h...@citrix.com>
Acked-by: Christian Lindig <christian.lin...@citrix.com>
>
> 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
>
