Xen project Mailing List

On Fri, Sep 6, 2024 at 2:38 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 03/09/2024 12:44 pm, Andrii Sultanov wrote:
> This plugin intends to hide the unstable Xenctrl interface under a
> stable one. In case of the change in the interface, a V2 of this plugin
> would need to be produced, but V1 with the old interface would
> need to be kept (with potential change in the implementation) in the
> meantime.
>
> To reduce the need for such changes in the future, this plugin only
> provides the absolute minimum functionality that Oxenstored uses - only
> three fields of the domaininfo struct are used and presented here.
>
> Oxenstored currently uses the single-domain domain_getinfo function,
> whereas Cxenstored uses the more-efficient domain_getinfolist. Both of
> these are provided in the plugin to allow a transition from one to the
> other without modifying the interface in the future. Both return
> identical structures and rely on the same fields in xenctrl, thus if one
> of them breaks, both will break, and a new version of the interface would
> need to be issued.
>
> Signed-off-by: Andrii Sultanov <andrii.sultanov@xxxxxxxxx>
> ---

Normally you should put a short set of notes here (under --- in the
commit message) about what has changed from the prior version you posted.

> tools/ocaml/Makefile | 1 +
> tools/ocaml/libs/Makefile | 2 +-
> tools/ocaml/libs/xenstoredglue/META.in | 4 +
> tools/ocaml/libs/xenstoredglue/Makefile | 46 +++++
> .../domain_getinfo_plugin_v1/META.in | 5 +
> .../domain_getinfo_plugin_v1/Makefile | 38 ++++
> .../domain_getinfo_stubs_v1.c | 172 ++++++++++++++++++
> .../domain_getinfo_v1.ml | 36 ++++
> .../domain_getinfo_v1.mli | 0
> .../libs/xenstoredglue/plugin_interface_v1.ml | 28 +++
> .../xenstoredglue/plugin_interface_v1.mli | 36 ++++
> 11 files changed, 367 insertions(+), 1 deletion(-)
> create mode 100644 tools/ocaml/libs/xenstoredglue/META.in
> create mode 100644 tools/ocaml/libs/xenstoredglue/Makefile
> create mode 100644 tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/META.in
> create mode 100644 tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/Makefile
> create mode 100644 tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_stubs_v1.c
> create mode 100644 tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.ml
> create mode 100644 tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.mli
> create mode 100644 tools/ocaml/libs/xenstoredglue/plugin_interface_v1.ml
> create mode 100644 tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli

We still have a mix of names even within this patch. xenstoredglue,
xenstored_glue, xsglue

Could we see about using xsd_glue as the top level name here, to get it
a bit shorter and easier to read?

> diff --git a/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/Makefile b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/Makefile
> new file mode 100644
> index 0000000000..9c40026cab
> --- /dev/null
> +++ b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/Makefile
> @@ -0,0 +1,38 @@
> +OCAML_TOPLEVEL=$(CURDIR)/../../..
> +XEN_ROOT=$(OCAML_TOPLEVEL)/../..
> +include $(OCAML_TOPLEVEL)/common.make
> +
> +CFLAGS += -I $(OCAML_TOPLEVEL)/libs -I $(OCAML_TOPLEVEL)/libs/xenstoredglue
> +CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_xeninclude) $(APPEND_CFLAGS)
> +OCAMLOPTFLAGS += -opaque
> +OCAMLINCLUDE += -I ./ -I ../

I think we only need "-I ../" here. xen-caml-compat.h is the only local
header used.

> diff --git a/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_stubs_v1.c b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_stubs_v1.c
> new file mode 100644
> index 0000000000..69eddd6412
> --- /dev/null
> +++ b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_stubs_v1.c
> @@ -0,0 +1,172 @@

Sorry for missing this before, but new files want to contain SDPX
headers. For this,

/* SPDX-License-Identifier: LGPL-2.1-only WITH
OCaml-LGPL-linking-exception */

which I had to go and research when sorting out xen-caml-compat.h

For Ocaml files, suppose we want.

(* SPDX-License-Identifier: LGPL-2.1-only WITH
OCaml-LGPL-linking-exception *)

The SPDX header should always be the first line of the file.

> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#define CAML_NAME_SPACE
> +#include <caml/alloc.h>
> +#include <caml/memory.h>
> +#include <caml/signals.h>
> +#include <caml/fail.h>
> +#include <caml/callback.h>
> +#include <caml/custom.h>
> +
> +#include <xen-tools/common-macros.h>
> +#include <xenctrl.h>
> +
> +#include "xen-caml-compat.h"
> +
> +#define ERR_MSG_LEN (XC_MAX_ERROR_MSG_LEN + 6)
> +#define MAX_FUNC_LINE_LEN 64

These two are obsolete given the rework of xsglue_failwith_xc()

> +#define failwith_xc_v1(xch) xsglue_failwith_xc(xch, __FUNCTION__, __LINE__)

This should be moved lower and adjusted. See later.

> +
> +/* This is a minimal stub to xenctrl for oxenstored's purposes
> + For the full xenctrl stubs, see tools/ocaml/libs/xc/xenctrl_stubs.c */

These comments aren't helpful IMO. It's said many times elsewhere.

> +
> +static inline xc_interface *xsglue_xch_of_val_v1(value v)

static functions don't a _v1 suffix, seeing as they're local to a file
with a _v1 in it's name.

> +{
> + xc_interface *xch = *(xc_interface **)Data_custom_val(v);
> +
> + return xch;
> +}
> +
> +static void xsglue_xenctrl_finalize(value v)
> +{
> + xc_interface *xch = xsglue_xch_of_val_v1(v);
> +
> + xc_interface_close(xch);
> +}
> +
> +static struct custom_operations xsglue_xenctrl_ops = {
> + .identifier = "xenctrl",

I presume this identifier gets elsewhere? Perhaps
"xsd_glue.domain_getinfo_v1.xenctrl" or so?

> + .finalize = xsglue_xenctrl_finalize,
> + .compare = custom_compare_default, /* Can't compare */
> + .hash = custom_hash_default, /* Can't hash */
> + .serialize = custom_serialize_default, /* Can't serialize */
> + .deserialize = custom_deserialize_default, /* Can't deserialize */
> + .compare_ext = custom_compare_ext_default, /* Can't compare */
> +};
> +

There's a trick with the C preprocessor where you can

#define xsglue_failwith(xch) xsglue_failwith(xch, __func__, __LINE__)

to add extra arguments to callsites. The only requirement is that it
either needs to be after the function of the same name, or that you
define the function using:

static void Noreturn (xsglue_failwith)(xc_interface *xch ...)

I'd put the macro and the declaration together here. Also, use __func__
rather than __FUNCTION__.

> +static void Noreturn xsglue_failwith_xc(xc_interface *xch,
> + const char* func,
> + unsigned int line)

The _xc() suffix isn't very helpful when there's also an xsglue_ prefix.

I'd simply name this xsglue_failwith(...) which is clear enough when used.

Also, 'const char *fun' with the * to the right of the space.

> +{
> + const xc_error *error = xch ? xc_get_last_error(xch) : NULL;
> + char *str = NULL;
> + CAMLparam0();
> + CAMLlocal1(msg);

Mixing tabs and spaces.

> <snip>
>
> +static value xsglue_alloc_domaininfo_v1(const xc_domaininfo_t *info)
> +{
> + CAMLparam0();
> + CAMLlocal1(result);

We try to split declarations from regular logic, so a blank line here
please.

> + result = caml_alloc_tuple(4);
> +
> + Store_field(result, 0, Val_int(info->domain));
> + Store_field(result, 1, Val_bool(info->flags & XEN_DOMINF_dying));
> + Store_field(result, 2, Val_bool(info->flags & XEN_DOMINF_shutdown));
> + Store_field(result, 3, Val_int(MASK_EXTR(info->flags, XEN_DOMINF_shutdownmask)));
> +
> + CAMLreturn(result);
> +}
> +
> +CAMLprim value stub_xsglue_xc_domain_getinfo(value xch_val, value domid)
> +{
> + CAMLparam2(xch_val, domid);
> + CAMLlocal1(result);
> + xc_interface *xch = xsglue_xch_of_val_v1(xch_val);
> + xc_domaininfo_t info;
> + int ret, domid_c;
> +
> + domid_c = Int_val(domid);

I'd suggests a blank line here, or to initialise domid_c at declaration.

> + caml_enter_blocking_section();
> + ret = xc_domain_getinfo_single(xch, domid_c, &info);
> + caml_leave_blocking_section();
> +
> + if (ret < 0)
> + failwith_xc_v1(xch);
> +
> + result = xsglue_alloc_domaininfo_v1(&info);
> +
> + CAMLreturn(result);
> +}
> +
> +CAMLprim value stub_xsglue_xc_domain_getinfolist(value xch_val, value first_domain)
> +{
> + CAMLparam2(xch_val, first_domain);
> + CAMLlocal1(result);
> + xc_interface *xch = xsglue_xch_of_val_v1(xch_val);
> + xc_domaininfo_t *info;
> + int i, ret, toalloc, retval;
> + uint32_t num_domains;
> + uint32_t c_first_domain;
> +
> + /* get the minimum number of allocate byte we need and bump it up to page boundary */
> + c_first_domain = Int_val(first_domain);

Passing a first_domain of anything other than 0 breaks the atomicity
that we're trying to create by asking for all domains together.

It wants dropping from here, and the plugin interface.

> + num_domains = DOMID_FIRST_RESERVED - c_first_domain;
> + toalloc = (sizeof(xc_domaininfo_t) * num_domains) | 0xfff;
> + ret = posix_memalign((void **) ((void *) &info), 4096, toalloc);

This is nonsense, and appears to have been so for ages in the Xenctrl stubs.

xc_domain_getinfolist() bounce-buffers the array anyway (to get
hypercall-safe buffers from the kernel), so there's no point doing any
local trickery. Simply:

    info = malloc(sizeof(xc_domaininfo_t) * DOMID_FIRST_RESERVED);
    if ( !info )
        caml_raise_out_of_memory();

will be fine.

> + if (ret)
> + caml_raise_out_of_memory();
> +
> + caml_enter_blocking_section();
> + retval = xc_domain_getinfolist(xch, c_first_domain, num_domains, info);
> + caml_leave_blocking_section();
> +
> + if (retval < 0) {
> + free(info);
> + failwith_xc_v1(xch);
> + } else if (retval > 0) {
> + result = caml_alloc(retval, 0);
> + for (i = 0; i < retval; i++) {
> + caml_modify(&Field(result, i), xsglue_alloc_domaininfo_v1(info + i));
> + }
> + } else {
> + result = Atom(0);
> + }

While this works, there are better ways of writing the logic.
failwith() is Noreturn, so it's easier to follow as:

    if (retval < 0) {
        ...
    }

    if (retval > 0) {
        ...

but... You're dom0, asking for all domains. Getting a retval of 0 is
also some kind of error, so I'd suggest:

    if (retval <= 0) {
        free(info);
        xsglue_failwith(xch);
    }

    result = caml_alloc(retval, 0);
    for (i = 0; i < retval; i++) {
        caml_modify(&Field(result, i), xsglue_alloc_domaininfo_v1(info +
i));
    }

    free(info);
    Camlreturn(result);

which is simpler still.

> diff --git a/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli b/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli
> new file mode 100644
> index 0000000000..d073a44d0f
> --- /dev/null
> +++ b/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli
> @@ -0,0 +1,36 @@
> +(** To avoid breaking the plugin interface, this module needs to be
> + standalone and can't rely on any other Xen library. Even unrelated
> + changes in the interfaces of those modules would change the hash
> + of this interface and break the plugin system.
> + It can only depend on Stdlib, therefore all of the types (domid,
> + domaininfo etc.) are redefined here instead of using alternatives
> + defined elsewhere.
> +
> + NOTE: The signature of this interface should not be changed (no
> + functions or types can be added, modified, or removed). If
> + underlying Xenctrl changes require a new interface, a V2 with a
> + corresponding plugin should be created.
> + *)

There's a rune to run ocp-indent in the Xen tree, in lieu of the full
Ocaml dev stack.

make -C tools/ocaml format

and the delta for this patch is just:

--- a/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli
+++ b/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.mli
@@ -10,7 +10,7 @@
     functions or types can be added, modified, or removed). If
     underlying Xenctrl changes require a new interface, a V2 with a
     corresponding plugin should be created.
- *)
+*)

module type Domain_getinfo_V1 = sig
   exception Error of string

~Andrew

Re: [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo