| Message ID | 20230117135336.11662-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: Switch to non-truncating XENVER_* ops | expand |
> On 17 Jan 2023, at 13:53, Andrew Cooper <andrew.cooper3@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.cooper3@citrix.com> > --- > CC: Wei Liu <wl@xen.org> > CC: Anthony PERARD <anthony.perard@citrix.com> > CC: Juergen Gross <jgross@suse.com> > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Edwin Torok <edvin.torok@citrix.com> > CC: Rob Hoes <Rob.Hoes@citrix.com> Acked-by: Christian Lindig <christian.lindig@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 >
On Tue, Jan 17, 2023 at 01:53:32PM +0000, Andrew Cooper 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@citrix.com> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
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); }
... 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@citrix.com> --- CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> CC: Juergen Gross <jgross@suse.com> CC: Christian Lindig <christian.lindig@citrix.com> CC: David Scott <dave@recoil.org> CC: Edwin Torok <edvin.torok@citrix.com> CC: Rob Hoes <Rob.Hoes@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(-)