* [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries
2024-09-03 11:44 [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Andrii Sultanov
@ 2024-09-03 11:44 ` Andrii Sultanov
2024-09-03 16:40 ` Andrew Cooper
2024-09-03 11:44 ` [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo Andrii Sultanov
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Andrii Sultanov @ 2024-09-03 11:44 UTC (permalink / raw)
To: xen-devel; +Cc: Andrii Sultanov, Christian Lindig, David Scott, Anthony PERARD
Dynamic libraries in OCaml require an additional compilation step on top
of the already specified steps for static libraries. Add an appropriate
template to Makefile.rules.
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
---
tools/ocaml/Makefile.rules | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index 5d534d8754..b9d4b51f0a 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -51,12 +51,13 @@ ifneq ($(MAKECMDGOALS),clean)
endif
clean: $(CLEAN_HOOKS)
- $(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
+ $(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.cmxs *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
distclean: clean
quiet-command = $(if $(V),$1,@printf " %-8s %s\n" "$2" "$3" && $1)
+mk-caml-shared-lib-native = $(call quiet-command, $(OCAMLOPT) $(OCAMLOPTFLAGS) -shared -linkall -o $1 $2 $3,MLA,$1)
mk-caml-lib-native = $(call quiet-command, $(OCAMLOPT) $(OCAMLOPTFLAGS) -a -o $1 $2 $3,MLA,$1)
mk-caml-lib-bytecode = $(call quiet-command, $(OCAMLC) $(OCAMLCFLAGS) -a -o $1 $2 $3,MLA,$1)
@@ -76,6 +77,19 @@ define OCAML_LIBRARY_template
$(call mk-caml-lib-stubs,$$@, $$+, $(foreach lib,$(LIBS_$(1)),$(lib)))
endef
+# Dynamically linked OCaml libraries ("plugins" in Dynlink parlance)
+# need to compile an .cmxs file
+define OCAML_DYN_LIBRARY_template
+ $(1).cmxs: $(1).cmxa
+ $(call mk-caml-shared-lib-native,$$@, $(1).cmxa)
+ $(1).cmxa: lib$(1)_stubs.a $(foreach obj,$($(1)_OBJS),$(obj).cmx)
+ $(call mk-caml-lib-native,$$@, -cclib -l$(1)_stubs $(foreach lib,$(LIBS_$(1)),-cclib $(lib)), $(foreach obj,$($(1)_OBJS),$(obj).cmx))
+ $(1)_stubs.a: $(foreach obj,$$($(1)_C_OBJS),$(obj).o)
+ $(call mk-caml-stubs,$$@, $$+)
+ lib$(1)_stubs.a: $(foreach obj,$($(1)_C_OBJS),$(obj).o)
+ $(call mk-caml-lib-stubs,$$@, $$+)
+endef
+
define OCAML_NOC_LIBRARY_template
$(1).cmxa: $(foreach obj,$($(1)_OBJS),$(obj).cmx)
$(call mk-caml-lib-native,$$@, , $(foreach obj,$($(1)_OBJS),$(obj).cmx))
@@ -98,6 +112,7 @@ endef
-include .ocamldep.make
$(foreach lib,$(OCAML_LIBRARY),$(eval $(call OCAML_LIBRARY_template,$(lib))))
+$(foreach lib,$(OCAML_DYN_LIBRARY),$(eval $(call OCAML_DYN_LIBRARY_template,$(lib))))
$(foreach lib,$(OCAML_NOC_LIBRARY),$(eval $(call OCAML_NOC_LIBRARY_template,$(lib))))
$(foreach p,$(OCAML_PROGRAM),$(eval $(call OCAML_PROGRAM_template,$(p))))
$(foreach p,$(C_PROGRAM),$(eval $(call C_PROGRAM_template,$(p))))
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries
2024-09-03 11:44 ` [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries Andrii Sultanov
@ 2024-09-03 16:40 ` Andrew Cooper
2024-09-04 8:27 ` Christian Lindig
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2024-09-03 16:40 UTC (permalink / raw)
To: Andrii Sultanov, xen-devel; +Cc: Christian Lindig, David Scott, Anthony PERARD
On 03/09/2024 12:44 pm, Andrii Sultanov wrote:
> Dynamic libraries in OCaml require an additional compilation step on top
> of the already specified steps for static libraries. Add an appropriate
> template to Makefile.rules.
>
> Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries
2024-09-03 16:40 ` Andrew Cooper
@ 2024-09-04 8:27 ` Christian Lindig
0 siblings, 0 replies; 12+ messages in thread
From: Christian Lindig @ 2024-09-04 8:27 UTC (permalink / raw)
To: Andrew Cooper
Cc: Andrii Sultanov, Xen-devel, Christian Lindig, David Scott,
Anthony PERARD
> On 3 Sep 2024, at 17:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 03/09/2024 12:44 pm, Andrii Sultanov wrote:
>> Dynamic libraries in OCaml require an additional compilation step on top
>> of the already specified steps for static libraries. Add an appropriate
>> template to Makefile.rules.
>>
>> Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo
2024-09-03 11:44 [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Andrii Sultanov
2024-09-03 11:44 ` [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries Andrii Sultanov
@ 2024-09-03 11:44 ` Andrii Sultanov
2024-09-06 13:38 ` Andrew Cooper
2024-09-06 14:23 ` Andrew Cooper
2024-09-03 11:44 ` [PATCH v2 3/3] tools/oxenstored: Use the " Andrii Sultanov
2024-09-06 12:02 ` [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Christian Lindig
3 siblings, 2 replies; 12+ messages in thread
From: Andrii Sultanov @ 2024-09-03 11:44 UTC (permalink / raw)
To: xen-devel; +Cc: Andrii Sultanov, Christian Lindig, David Scott, Anthony PERARD
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@cloud.com>
---
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
diff --git a/tools/ocaml/Makefile b/tools/ocaml/Makefile
index 1557fd6c3c..eb426f2ee5 100644
--- a/tools/ocaml/Makefile
+++ b/tools/ocaml/Makefile
@@ -29,6 +29,7 @@ build-tools-oxenstored:
$(MAKE) -s -C libs/mmap
$(MAKE) -s -C libs/xb
$(MAKE) -s -C libs/xc
+ $(MAKE) -s -C libs/xenstoredglue
$(MAKE) -C xenstored
.PHONY: format
diff --git a/tools/ocaml/libs/Makefile b/tools/ocaml/libs/Makefile
index 89350aa12f..828fbf859d 100644
--- a/tools/ocaml/libs/Makefile
+++ b/tools/ocaml/libs/Makefile
@@ -4,7 +4,7 @@ include $(XEN_ROOT)/tools/Rules.mk
SUBDIRS= \
mmap \
eventchn xc \
- xb xs
+ xb xs xenstoredglue
.PHONY: all
all: subdirs-all
diff --git a/tools/ocaml/libs/xenstoredglue/META.in b/tools/ocaml/libs/xenstoredglue/META.in
new file mode 100644
index 0000000000..dbd584ac17
--- /dev/null
+++ b/tools/ocaml/libs/xenstoredglue/META.in
@@ -0,0 +1,4 @@
+version = "@VERSION@"
+description = "A small library on top of unstable Xenctrl interfaces used by Oxenstored"
+archive(byte) = "plugin_interface_v1.cma"
+archive(native) = "plugin_interface_v1.cmxa"
diff --git a/tools/ocaml/libs/xenstoredglue/Makefile b/tools/ocaml/libs/xenstoredglue/Makefile
new file mode 100644
index 0000000000..95818cdf83
--- /dev/null
+++ b/tools/ocaml/libs/xenstoredglue/Makefile
@@ -0,0 +1,46 @@
+OCAML_TOPLEVEL=$(CURDIR)/../..
+XEN_ROOT=$(OCAML_TOPLEVEL)/../..
+include $(OCAML_TOPLEVEL)/common.make
+
+SUBDIRS= domain_getinfo_plugin_v1
+
+CFLAGS += $(CFLAGS_xeninclude)
+OCAMLOPTFLAGS += -opaque
+
+OBJS = plugin_interface_v1
+INTF = $(foreach obj, $(OBJS),$(obj).cmi)
+LIBS = plugin_interface_v1.cma plugin_interface_v1.cmxa
+LIBS_plugin_interface_v1 =
+plugin_interface_v1_OBJS=$(OBJS)
+
+.PHONY: all
+all: $(INTF) $(LIBS) $(PROGRAMS) subdirs-all
+
+bins: $(PROGRAMS)
+
+libs: $(LIBS)
+
+plugin_interface_v1 = $(OBJS)
+
+OCAML_NOC_LIBRARY = plugin_interface_v1
+
+.PHONY: install
+install: $(LIBS) META subdirs-install
+ mkdir -p $(OCAMLDESTDIR)
+ $(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstored_glue
+ $(OCAMLFIND) install -destdir $(OCAMLDESTDIR) -ldconf ignore xenstored_glue META $(INTF) $(LIBS)
+ $(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstored_glue_dev
+ $(OCAMLFIND) install -destdir $(OCAMLDESTDIR) -ldconf ignore xenstored_glue_dev META $(INTF) $(LIBS) *.ml *.mli
+
+.PHONY: uninstall
+uninstall: subdirs-uninstall
+ $(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstored_glue
+ $(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstored_glue_dev
+
+.PHONY: clean
+clean: subdirs-clean
+
+.PHONY: distclean
+distclean: subdirs-distclean
+
+include $(OCAML_TOPLEVEL)/Makefile.rules
diff --git a/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/META.in b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/META.in
new file mode 100644
index 0000000000..fb917def62
--- /dev/null
+++ b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/META.in
@@ -0,0 +1,5 @@
+version = "@VERSION@"
+description = "Xenstored plugin for Xenctrl.domain_getinfo unstable interface - V1"
+requires = "plugin_interface_v1"
+archive(byte) = "domain_getinfo_v1.cma"
+archive(native) = "domain_getinfo_v1.cmxa"
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 ../
+
+OBJS = domain_getinfo_v1
+INTF = $(foreach obj, $(OBJS),$(obj).cmi)
+LIBS = domain_getinfo_v1.cmxa domain_getinfo_v1.cmxs
+
+LIBS_xenstoredglue = $(call xenlibs-ldflags-ldlibs,xenctrl)
+
+all: $(INTF) $(LIBS) $(PROGRAMS)
+
+bins: $(PROGRAMS)
+
+libs: $(LIBS)
+
+domain_getinfo_v1_OBJS = $(OBJS)
+domain_getinfo_v1 = $(OBJS)
+domain_getinfo_v1_C_OBJS = domain_getinfo_stubs_v1
+
+OCAML_DYN_LIBRARY = domain_getinfo_v1
+
+.PHONY: install
+install: $(LIBS) META
+ $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)/xenstored_glue/xenctrl_plugin
+ $(INSTALL_PROG) domain_getinfo_v1.cmxs $(DESTDIR)$(LIBEXEC_BIN)/xenstored_glue/xenctrl_plugin
+
+.PHONY: uninstall
+uninstall:
+ rm -f $(DESTDIR)/xenstored_glue/xenctrl_plugin/domain_getinfo_v1.cmxs
+
+include $(OCAML_TOPLEVEL)/Makefile.rules
+
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 @@
+#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
+#define failwith_xc_v1(xch) xsglue_failwith_xc(xch, __FUNCTION__, __LINE__)
+
+/* This is a minimal stub to xenctrl for oxenstored's purposes
+ For the full xenctrl stubs, see tools/ocaml/libs/xc/xenctrl_stubs.c */
+
+static inline xc_interface *xsglue_xch_of_val_v1(value v)
+{
+ 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",
+ .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 */
+};
+
+static void Noreturn xsglue_failwith_xc(xc_interface *xch,
+ const char* func,
+ unsigned int line)
+{
+ const xc_error *error = xch ? xc_get_last_error(xch) : NULL;
+ char *str = NULL;
+ CAMLparam0();
+ CAMLlocal1(msg);
+
+#define ERR (error && error->code != XC_ERROR_NONE)
+
+ int ret = asprintf(&str,
+ "%d: %s%s%s - called from %s:%u",
+ ERR ? error->code : errno,
+ ERR ? xc_error_code_to_desc(error->code) : strerror(errno),
+ ERR ? ": " : "",
+ ERR ? error->message : "",
+ func, line);
+
+#undef ERR
+
+ if (!*str || (ret == -1))
+ caml_raise_out_of_memory();
+
+ msg = caml_copy_string(str);
+ free(str);
+
+ caml_raise_with_arg(*caml_named_value("xsg.error_v1"), msg);
+}
+
+CAMLprim value stub_xsglue_xc_interface_open(value unit)
+{
+ CAMLparam1(unit);
+ CAMLlocal1(result);
+ xc_interface *xch;
+
+ result = caml_alloc_custom(&xsglue_xenctrl_ops, sizeof(xch), 0, 1);
+
+ caml_enter_blocking_section();
+ xch = xc_interface_open(NULL, NULL, 0);
+ caml_leave_blocking_section();
+
+ if (!xch)
+ failwith_xc_v1(xch);
+
+ *(xc_interface **)Data_custom_val(result) = xch;
+
+ CAMLreturn(result);
+}
+
+static value xsglue_alloc_domaininfo_v1(const xc_domaininfo_t *info)
+{
+ CAMLparam0();
+ CAMLlocal1(result);
+ 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);
+ 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);
+ num_domains = DOMID_FIRST_RESERVED - c_first_domain;
+ toalloc = (sizeof(xc_domaininfo_t) * num_domains) | 0xfff;
+ ret = posix_memalign((void **) ((void *) &info), 4096, toalloc);
+ 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);
+ }
+
+ free(info);
+ CAMLreturn(result);
+}
diff --git a/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.ml b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.ml
new file mode 100644
index 0000000000..6f282e4257
--- /dev/null
+++ b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.ml
@@ -0,0 +1,36 @@
+(** Minimal interface on top of unstable Xenctrl for Oxenstored's usage *)
+
+(** For the full Xenctrl interface, see: tools/ocaml/libs/xc/ *)
+
+module P = Plugin_interface_v1
+
+module M : P.Domain_getinfo_V1 = struct
+ exception Error of string
+
+ type domid = int
+ type handle
+
+ type domaininfo = {
+ domid : domid;
+ dying : bool;
+ shutdown : bool;
+ shutdown_code : int;
+ }
+
+ external interface_open : unit -> handle = "stub_xsglue_xc_interface_open"
+
+ external domain_getinfo : handle -> domid -> domaininfo
+ = "stub_xsglue_xc_domain_getinfo"
+
+ external domain_getinfolist : handle -> domid -> domaininfo array
+ = "stub_xsglue_xc_domain_getinfolist"
+
+ let _ = Callback.register_exception "xsg.error_v1" (Error "register_callback")
+end
+
+let () =
+ Printf.ksprintf !P.logging_function "Registration of %s plugin started\n%!"
+ __MODULE__;
+ P.register_plugin_v1 (module M : P.Domain_getinfo_V1);
+ Printf.ksprintf !P.logging_function "Registration of %s plugin successful\n%!"
+ __MODULE__
diff --git a/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.mli b/tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.mli
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.ml b/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.ml
new file mode 100644
index 0000000000..131b29d3ee
--- /dev/null
+++ b/tools/ocaml/libs/xenstoredglue/plugin_interface_v1.ml
@@ -0,0 +1,28 @@
+module type Domain_getinfo_V1 = sig
+ exception Error of string
+
+ type domid = int
+ type handle
+
+ type domaininfo = {
+ domid : domid;
+ dying : bool;
+ shutdown : bool;
+ shutdown_code : int;
+ }
+
+ val interface_open : unit -> handle
+ val domain_getinfo : handle -> domid -> domaininfo
+ val domain_getinfolist : handle -> domid -> domaininfo array
+end
+
+let ignore_logging : string -> unit = ignore
+let logging_function = ref ignore_logging
+let register_logging_function func = logging_function := func
+let plugin_implementation_v1 : (module Domain_getinfo_V1) option ref = ref None
+let register_plugin_v1 m = plugin_implementation_v1 := Some m
+
+let get_plugin_v1 () : (module Domain_getinfo_V1) =
+ match !plugin_implementation_v1 with
+ | Some s -> s
+ | None -> failwith "No plugin loaded"
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.
+ *)
+
+module type Domain_getinfo_V1 = sig
+ exception Error of string
+
+ type domid = int
+ type handle
+
+ type domaininfo = {
+ domid : domid;
+ dying : bool;
+ shutdown : bool;
+ shutdown_code : int;
+ }
+
+ val interface_open : unit -> handle
+ val domain_getinfo : handle -> domid -> domaininfo
+ val domain_getinfolist : handle -> domid -> domaininfo array
+end
+
+val register_logging_function : (string -> unit) -> unit
+val logging_function : (string -> unit) ref
+val register_plugin_v1 : (module Domain_getinfo_V1) -> unit
+val get_plugin_v1 : unit -> (module Domain_getinfo_V1)
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo
2024-09-03 11:44 ` [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo Andrii Sultanov
@ 2024-09-06 13:38 ` Andrew Cooper
2024-09-06 14:20 ` Andrii Sultanov
2024-09-06 14:23 ` Andrew Cooper
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2024-09-06 13:38 UTC (permalink / raw)
To: Andrii Sultanov, xen-devel; +Cc: Christian Lindig, David Scott, Anthony PERARD
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@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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo
2024-09-06 13:38 ` Andrew Cooper
@ 2024-09-06 14:20 ` Andrii Sultanov
2024-09-06 14:31 ` Andrew Cooper
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Sultanov @ 2024-09-06 14:20 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Christian Lindig, David Scott, Anthony PERARD
[-- Attachment #1: Type: text/plain, Size: 14993 bytes --]
> > 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.cooper3@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.sultanov@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
>
[-- Attachment #2: Type: text/html, Size: 18027 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo
2024-09-06 14:20 ` Andrii Sultanov
@ 2024-09-06 14:31 ` Andrew Cooper
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-09-06 14:31 UTC (permalink / raw)
To: Andrii Sultanov; +Cc: xen-devel, Christian Lindig, David Scott, Anthony PERARD
On 06/09/2024 3:20 pm, Andrii Sultanov wrote:
>> > 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
> ```
Oh ok. Better keep it then.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo
2024-09-03 11:44 ` [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo Andrii Sultanov
2024-09-06 13:38 ` Andrew Cooper
@ 2024-09-06 14:23 ` Andrew Cooper
1 sibling, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-09-06 14:23 UTC (permalink / raw)
To: Andrii Sultanov, xen-devel; +Cc: Christian Lindig, David Scott, Anthony PERARD
On 03/09/2024 12:44 pm, Andrii Sultanov wrote:
> 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
Only noticed after some trial builds.
This patch should include tools/ocaml/libs/xenstoredglue/.gitignore
which causes these paths:
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/.ocamldep.make
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.annot
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.cmi
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.cmx
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.cmxa
tools/ocaml/libs/xenstoredglue/domain_getinfo_plugin_v1/domain_getinfo_v1.cmxs
to get ignored. Although, I don't see any local gitignores, so I
suspect it's still the root .gitignore which wants adjusting.
I'll see about doing another cleanup patch.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] tools/oxenstored: Use the plugin for Xenctrl.domain_getinfo
2024-09-03 11:44 [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Andrii Sultanov
2024-09-03 11:44 ` [PATCH v2 1/3] tools/ocaml: Build infrastructure for OCaml dynamic libraries Andrii Sultanov
2024-09-03 11:44 ` [PATCH v2 2/3] ocaml/libs: Implement a dynamically-loaded plugin for Xenctrl.domain_getinfo Andrii Sultanov
@ 2024-09-03 11:44 ` Andrii Sultanov
2024-09-06 14:30 ` Andrew Cooper
2024-09-06 12:02 ` [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Christian Lindig
3 siblings, 1 reply; 12+ messages in thread
From: Andrii Sultanov @ 2024-09-03 11:44 UTC (permalink / raw)
To: xen-devel
Cc: Andrii Sultanov, Andrew Cooper, Jan Beulich, Julien Grall,
Stefano Stabellini, Anthony PERARD, Christian Lindig, David Scott
Oxenstored dynamically loads the plugin provided in
ocaml/libs/xenstoredglue.
The plugin is verified to be providing the specified plugin_interface
during its loading.
If a V2 of the plugin is produced, V1 will still be present, and a new
version should only be loaded if it's verified to exist
(New oxenstored can run in an environment with only V1 of the plugin).
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
---
Config.mk | 2 +-
configure | 7 ++++
m4/paths.m4 | 4 ++
tools/configure | 7 ++++
tools/ocaml/xenstored/Makefile | 5 ++-
tools/ocaml/xenstored/domains.ml | 63 +++++++++++++++++++++----------
tools/ocaml/xenstored/paths.ml.in | 1 +
7 files changed, 68 insertions(+), 21 deletions(-)
diff --git a/Config.mk b/Config.mk
index 1a3938b6c4..4be7d8a573 100644
--- a/Config.mk
+++ b/Config.mk
@@ -160,7 +160,7 @@ endef
BUILD_MAKE_VARS := sbindir bindir LIBEXEC LIBEXEC_BIN libdir SHAREDIR \
XENFIRMWAREDIR XEN_CONFIG_DIR XEN_SCRIPT_DIR XEN_LOCK_DIR \
XEN_RUN_DIR XEN_PAGING_DIR XEN_DUMP_DIR XEN_LOG_DIR \
- XEN_LIB_DIR XEN_RUN_STORED
+ XEN_LIB_DIR XEN_RUN_STORED XEN_CTRL_DOMAININFO_PLUGIN
buildmakevars2file = $(eval $(call buildmakevars2file-closure,$(1)))
define buildmakevars2file-closure
diff --git a/configure b/configure
index 2d7b20aa50..20aae12884 100755
--- a/configure
+++ b/configure
@@ -631,6 +631,7 @@ XEN_PAGING_DIR
XEN_LOCK_DIR
INITD_DIR
SHAREDIR
+XEN_CTRL_DOMAININFO_PLUGIN
XEN_LIB_DIR
XEN_RUN_STORED
XEN_LOG_DIR
@@ -2199,6 +2200,12 @@ XEN_LIB_DIR=$localstatedir/lib/xen
printf "%s\n" "#define XEN_LIB_DIR \"$XEN_LIB_DIR\"" >>confdefs.h
+XEN_CTRL_DOMAININFO_PLUGIN=$LIBEXEC_BIN/xenstored_glue/xenctrl_plugin
+
+
+printf "%s\n" "#define XEN_CTRL_DOMAININFO_PLUGIN \"$XEN_CTRL_DOMAININFO_PLUGIN\"" >>confdefs.h
+
+
SHAREDIR=$prefix/share
diff --git a/m4/paths.m4 b/m4/paths.m4
index 3f94c62efb..533bac919b 100644
--- a/m4/paths.m4
+++ b/m4/paths.m4
@@ -144,6 +144,10 @@ XEN_LIB_DIR=$localstatedir/lib/xen
AC_SUBST(XEN_LIB_DIR)
AC_DEFINE_UNQUOTED([XEN_LIB_DIR], ["$XEN_LIB_DIR"], [Xen's lib dir])
+XEN_CTRL_DOMAININFO_PLUGIN=$LIBEXEC_BIN/xenstored_glue/xenctrl_plugin
+AC_SUBST(XEN_CTRL_DOMAININFO_PLUGIN)
+AC_DEFINE_UNQUOTED([XEN_CTRL_DOMAININFO_PLUGIN], ["$XEN_CTRL_DOMAININFO_PLUGIN"], [Xenctrl's plugin for Oxenstored])
+
SHAREDIR=$prefix/share
AC_SUBST(SHAREDIR)
diff --git a/tools/configure b/tools/configure
index 7f98303fdd..830c8c1533 100755
--- a/tools/configure
+++ b/tools/configure
@@ -743,6 +743,7 @@ XEN_PAGING_DIR
XEN_LOCK_DIR
INITD_DIR
SHAREDIR
+XEN_CTRL_DOMAININFO_PLUGIN
XEN_LIB_DIR
XEN_RUN_STORED
XEN_LOG_DIR
@@ -4530,6 +4531,12 @@ XEN_LIB_DIR=$localstatedir/lib/xen
printf "%s\n" "#define XEN_LIB_DIR \"$XEN_LIB_DIR\"" >>confdefs.h
+XEN_CTRL_DOMAININFO_PLUGIN=$LIBEXEC_BIN/xenstored_glue/xenctrl_plugin
+
+
+printf "%s\n" "#define XEN_CTRL_DOMAININFO_PLUGIN \"$XEN_CTRL_DOMAININFO_PLUGIN\"" >>confdefs.h
+
+
SHAREDIR=$prefix/share
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index fa45305d8c..09896732ed 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -15,7 +15,8 @@ OCAMLINCLUDE += \
-I $(OCAML_TOPLEVEL)/libs/xb \
-I $(OCAML_TOPLEVEL)/libs/mmap \
-I $(OCAML_TOPLEVEL)/libs/xc \
- -I $(OCAML_TOPLEVEL)/libs/eventchn
+ -I $(OCAML_TOPLEVEL)/libs/eventchn \
+ -I $(OCAML_TOPLEVEL)/libs/xenstoredglue
LIBS = syslog.cma syslog.cmxa poll.cma poll.cmxa
syslog_OBJS = syslog
@@ -59,6 +60,7 @@ INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi poll.cmi
XENSTOREDLIBS = \
unix.cmxa \
+ dynlink.cmxa \
-ccopt -L -ccopt . syslog.cmxa \
-ccopt -L -ccopt . systemd.cmxa \
-ccopt -L -ccopt . poll.cmxa \
@@ -66,6 +68,7 @@ XENSTOREDLIBS = \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn $(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xc $(OCAML_TOPLEVEL)/libs/xc/xenctrl.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xb $(OCAML_TOPLEVEL)/libs/xb/xenbus.cmxa \
+ -ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xenstoredglue $(OCAML_TOPLEVEL)/libs/xenstoredglue/plugin_interface_v1.cmxa \
-ccopt -L -ccopt $(XEN_ROOT)/tools/libs/ctrl
PROGRAMS = oxenstored
diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml
index 7a3056c364..dfff84c918 100644
--- a/tools/ocaml/xenstored/domains.ml
+++ b/tools/ocaml/xenstored/domains.ml
@@ -20,10 +20,36 @@ let warn fmt = Logging.warn "domains" fmt
let xc = Xenctrl.interface_open ()
-type domains = {
- eventchn: Event.t;
- table: (Xenctrl.domid, Domain.t) Hashtbl.t;
+let load_plug fname =
+ let fail_with fmt = Printf.ksprintf failwith fmt in
+ let fname = Dynlink.adapt_filename fname in
+ if Sys.file_exists fname then
+ try Dynlink.loadfile fname with
+ | Dynlink.Error err as e ->
+ error "ERROR loading plugin '%s': %s\n%!" fname
+ (Dynlink.error_message err);
+ raise e
+ | _ -> fail_with "Unknown error while loading plugin"
+ else fail_with "Plugin file '%s' does not exist" fname
+
+let () =
+ let filepath = Paths.xen_ctrl_plugin ^ "/domain_getinfo_v1.cmxs" in
+ debug "Trying to load plugin '%s'\n%!" filepath;
+ let list_files = Sys.readdir Paths.xen_ctrl_plugin in
+ debug "Directory listing of '%s'\n%!" Paths.xen_ctrl_plugin;
+ Array.iter (fun x -> debug "\t%s\n%!" x) list_files;
+ Dynlink.allow_only [ "Plugin_interface_v1" ];
+ load_plug filepath
+
+module Plugin =
+ (val Plugin_interface_v1.get_plugin_v1 ()
+ : Plugin_interface_v1.Domain_getinfo_V1)
+
+let handle = Plugin.interface_open ()
+type domains = {
+ eventchn : Event.t;
+ table : (Plugin.domid, Domain.t) Hashtbl.t;
(* N.B. the Queue module is not thread-safe but oxenstored is single-threaded. *)
(* Domains queue up to regain conflict-credit; we have a queue for
domains that are carrying some penalty and so are below the
@@ -93,22 +119,21 @@ let cleanup doms =
let notify = ref false in
let dead_dom = ref [] in
- Hashtbl.iter (fun id _ -> if id <> 0 then
- try
- let info = Xenctrl.domain_getinfo xc id in
- if info.Xenctrl.shutdown || info.Xenctrl.dying then (
- debug "Domain %u died (dying=%b, shutdown %b -- code %d)"
- id info.Xenctrl.dying info.Xenctrl.shutdown info.Xenctrl.shutdown_code;
- if info.Xenctrl.dying then
- dead_dom := id :: !dead_dom
- else
- notify := true;
- )
- with Xenctrl.Error _ ->
- debug "Domain %u died -- no domain info" id;
- dead_dom := id :: !dead_dom;
- ) doms.table;
- List.iter (fun id ->
+ Hashtbl.iter
+ (fun id _ ->
+ if id <> 0 then (
+ try
+ let info = Plugin.domain_getinfo handle id in
+ if info.Plugin.shutdown || info.Plugin.dying then (
+ debug "Domain %u died (dying=%b, shutdown %b -- code %d)" id
+ info.Plugin.dying info.Plugin.shutdown info.Plugin.shutdown_code;
+ if info.Plugin.dying then dead_dom := id :: !dead_dom else notify := true)
+ with Plugin.Error _ ->
+ debug "Domain %u died -- no domain info" id;
+ dead_dom := id :: !dead_dom))
+ doms.table;
+ List.iter
+ (fun id ->
let dom = Hashtbl.find doms.table id in
Domain.close dom;
Hashtbl.remove doms.table id;
diff --git a/tools/ocaml/xenstored/paths.ml.in b/tools/ocaml/xenstored/paths.ml.in
index 37949dc8f3..67276dda94 100644
--- a/tools/ocaml/xenstored/paths.ml.in
+++ b/tools/ocaml/xenstored/paths.ml.in
@@ -2,3 +2,4 @@ let xen_log_dir = "@XEN_LOG_DIR@"
let xen_config_dir = "@XEN_CONFIG_DIR@"
let xen_run_dir = "@XEN_RUN_DIR@"
let xen_run_stored = "@XEN_RUN_STORED@"
+let xen_ctrl_plugin = "@XEN_CTRL_DOMAININFO_PLUGIN@"
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/3] tools/oxenstored: Use the plugin for Xenctrl.domain_getinfo
2024-09-03 11:44 ` [PATCH v2 3/3] tools/oxenstored: Use the " Andrii Sultanov
@ 2024-09-06 14:30 ` Andrew Cooper
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-09-06 14:30 UTC (permalink / raw)
To: Andrii Sultanov, xen-devel
Cc: Jan Beulich, Julien Grall, Stefano Stabellini, Anthony PERARD,
Christian Lindig, David Scott
On 03/09/2024 12:44 pm, Andrii Sultanov wrote:
> diff --git a/m4/paths.m4 b/m4/paths.m4
> index 3f94c62efb..533bac919b 100644
> --- a/m4/paths.m4
> +++ b/m4/paths.m4
> @@ -144,6 +144,10 @@ XEN_LIB_DIR=$localstatedir/lib/xen
> AC_SUBST(XEN_LIB_DIR)
> AC_DEFINE_UNQUOTED([XEN_LIB_DIR], ["$XEN_LIB_DIR"], [Xen's lib dir])
>
> +XEN_CTRL_DOMAININFO_PLUGIN=$LIBEXEC_BIN/xenstored_glue/xenctrl_plugin
> +AC_SUBST(XEN_CTRL_DOMAININFO_PLUGIN)
> +AC_DEFINE_UNQUOTED([XEN_CTRL_DOMAININFO_PLUGIN], ["$XEN_CTRL_DOMAININFO_PLUGIN"], [Xenctrl's plugin for Oxenstored])
> +
This is somewhat complicated, and I'm not sure what to suggest.
As far as this patch goes, it's "where should Oxenstored look for it's
plugin in the target system"
But, with that intent, the prior patch's install rule needs to know
"where should ocaml plugins be put in the target system".
That said, it isn't really configurable. It's just a path formed of
other ./configure fragments, so IMO it would be better to not add a
toplevel new path. Just opencode it on the one line lower down, like
the install rule was in the previous patch.
Finally, looking at the XenServer spec, the path is:
%{_libexecdir}/%{name}/bin/xenstored_glue/xenctrl_plugin/domain_getinfo_v1.cmxs
Its not really /bin/ appropriate, so perhaps this:
%{_libexecdir}/%{name}/ocaml/xenstored_glue/xenctrl_plugin/domain_getinfo_v1.cmxs
which would mean that you just want $LIBEXEC as a base.
> diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml
> index 7a3056c364..dfff84c918 100644
> --- a/tools/ocaml/xenstored/domains.ml
> +++ b/tools/ocaml/xenstored/domains.ml
> @@ -20,10 +20,36 @@ let warn fmt = Logging.warn "domains" fmt
>
> let xc = Xenctrl.interface_open ()
>
> -type domains = {
> - eventchn: Event.t;
> - table: (Xenctrl.domid, Domain.t) Hashtbl.t;
> +let load_plug fname =
> + let fail_with fmt = Printf.ksprintf failwith fmt in
> + let fname = Dynlink.adapt_filename fname in
> + if Sys.file_exists fname then
> + try Dynlink.loadfile fname with
> + | Dynlink.Error err as e ->
> + error "ERROR loading plugin '%s': %s\n%!" fname
> + (Dynlink.error_message err);
> + raise e
> + | _ -> fail_with "Unknown error while loading plugin"
> + else fail_with "Plugin file '%s' does not exist" fname
> +
> +let () =
> + let filepath = Paths.xen_ctrl_plugin ^ "/domain_getinfo_v1.cmxs" in
> + debug "Trying to load plugin '%s'\n%!" filepath;
> + let list_files = Sys.readdir Paths.xen_ctrl_plugin in
> + debug "Directory listing of '%s'\n%!" Paths.xen_ctrl_plugin;
> + Array.iter (fun x -> debug "\t%s\n%!" x) list_files;
> + Dynlink.allow_only [ "Plugin_interface_v1" ];
> + load_plug filepath
> +
> +module Plugin =
> + (val Plugin_interface_v1.get_plugin_v1 ()
> + : Plugin_interface_v1.Domain_getinfo_V1)
> +
> +let handle = Plugin.interface_open ()
>
> +type domains = {
> + eventchn : Event.t;
> + table : (Plugin.domid, Domain.t) Hashtbl.t;
This will still be better split into two; one patch loading the plugin
and a separate patch switching to use it.
I've got a local branch with the split working (compiling), if you'd like.
That said, one reason why this diff is more complicated to read is that
you've deleted a blank line here, vs the old type
> (* N.B. the Queue module is not thread-safe but oxenstored is single-threaded. *)
> (* Domains queue up to regain conflict-credit; we have a queue for
> domains that are carrying some penalty and so are below the
> @@ -93,22 +119,21 @@ let cleanup doms =
> let notify = ref false in
> let dead_dom = ref [] in
>
> - Hashtbl.iter (fun id _ -> if id <> 0 then
> - try
> - let info = Xenctrl.domain_getinfo xc id in
> - if info.Xenctrl.shutdown || info.Xenctrl.dying then (
> - debug "Domain %u died (dying=%b, shutdown %b -- code %d)"
> - id info.Xenctrl.dying info.Xenctrl.shutdown info.Xenctrl.shutdown_code;
> - if info.Xenctrl.dying then
> - dead_dom := id :: !dead_dom
> - else
> - notify := true;
> - )
> - with Xenctrl.Error _ ->
> - debug "Domain %u died -- no domain info" id;
> - dead_dom := id :: !dead_dom;
> - ) doms.table;
> - List.iter (fun id ->
> + Hashtbl.iter
> + (fun id _ ->
> + if id <> 0 then (
> + try
> + let info = Plugin.domain_getinfo handle id in
> + if info.Plugin.shutdown || info.Plugin.dying then (
> + debug "Domain %u died (dying=%b, shutdown %b -- code %d)" id
> + info.Plugin.dying info.Plugin.shutdown info.Plugin.shutdown_code;
> + if info.Plugin.dying then dead_dom := id :: !dead_dom else notify := true)
> + with Plugin.Error _ ->
> + debug "Domain %u died -- no domain info" id;
> + dead_dom := id :: !dead_dom))
> + doms.table;
> + List.iter
> + (fun id ->
ocp-indent makes a number of changes to this block.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored
2024-09-03 11:44 [PATCH v2 0/3] tools/ocaml: Stabilize domain_getinfo for Oxenstored Andrii Sultanov
` (2 preceding siblings ...)
2024-09-03 11:44 ` [PATCH v2 3/3] tools/oxenstored: Use the " Andrii Sultanov
@ 2024-09-06 12:02 ` Christian Lindig
3 siblings, 0 replies; 12+ messages in thread
From: Christian Lindig @ 2024-09-06 12:02 UTC (permalink / raw)
To: Andrii Sultanov
Cc: Xen-devel, Christian Lindig, David Scott, Anthony PERARD,
Andrew Cooper, Jan Beulich, Julien Grall, Stefano Stabellini
There was some confusion about my ack for this series - my apologies. I intended to ack’ it entirely.
Acked-by: Christian Lindig <christian.lindig@cloud.com>
> On 3 Sep 2024, at 12:44, Andrii Sultanov <andrii.sultanov@cloud.com> wrote:
>
> This is a V2 of the "Stabilize Oxenstored's interface with" patch
> series, see V1's cover letter for the motivation of these changes.
>
> Two patches from V1 ("tools/ocaml: Fix OCaml libs rules" and
> "Remove '-cc $(CC)' from OCAMLOPTFLAGS") were commited upstream, so
> they've been dropped from here.
>
> V2 addresses the following review comments and suggestions:
>
> * Split the plugin implementation commit into two - build
> infrastructure-related and implementation itself.
> * Package xenstored_glue interface, since it's needed for an
> out-of-tree oxenstored to build. Additionally package xenstored_glue_dev
> with .ml and .mli files.
> * Clean up unnecessary #define, #include, and fix function parameter
> types as suggested.
> * Improve error handling in xsglue_failwith_xc, additionally version
> xsg.error to avoid potential future conflicts.
> * Drop the GC lock around xc_domain_getinfo_single, and move Int_val
> outside of the blocking_section.
> * Use existing logging facilities in oxenstored
> * Plugin now uses logging function provided by the plugin interface
> (ignoring input by default)
> * domain_getinfolist gets all 32k domains at once instead of listing them
> in chunks, storing information into an array. OCaml interface was simplified
> appropriately. (Cxenstored does not do this at the moment - as far as I
> understand, it also uses the single-domain version of the function).
>
> I've decided against introducing an enum for the shutdown code, as it adds
> additional complexity (and potential reasons for creating a new plugin version)
> without any benefit - oxenstored does not care about its value at the moment.
>
> Patch series on Gitlab for ease of review:
> https://gitlab.com/xen-project/people/asultanov/xen/-/compare/staging...plugin-v3
>
> V2 passed the Gitlab CI: https://gitlab.com/xen-project/people/asultanov/xen/-/pipelines/1437391764
> It was also tested on some hosts.
>
> Andrii Sultanov (3):
> tools/ocaml: Build infrastructure for OCaml dynamic libraries
> ocaml/libs: Implement a dynamically-loaded plugin for
> Xenctrl.domain_getinfo
> tools/oxenstored: Use the plugin for Xenctrl.domain_getinfo
>
> Config.mk | 2 +-
> configure | 7 +
> m4/paths.m4 | 4 +
> tools/configure | 7 +
> tools/ocaml/Makefile | 1 +
> tools/ocaml/Makefile.rules | 17 +-
> 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 ++++
> tools/ocaml/xenstored/Makefile | 5 +-
> tools/ocaml/xenstored/domains.ml | 63 +++++--
> tools/ocaml/xenstored/paths.ml.in | 1 +
> 19 files changed, 451 insertions(+), 23 deletions(-)
> 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
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 12+ messages in thread