> > 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.
With only "-I ../", the build fails:
```
ocamlopt -g -ccopt " " -dtypes -I ../ -w F -warn-error F -opaque -shared
-linkall -o domain_getinfo_v1.cmxs domain_getinfo_v1.cmxa
/usr/bin/ld: cannot find -ldomain_getinfo_v1_stubs: No such file or
directory
collect2: error: ld returned 1 exit status
```
Thank you very much for the rest, I will submit a new version of the patch
series shortly.
On Fri, Sep 6, 2024 at 2:38 PM Andrew Cooper <andrew.coop...@citrix.com>
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.sulta...@cloud.com>
> > ---
>
> 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
>
