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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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