| Message ID | ffd83cc13f440605337ff018c9b2ccdd3a4be0c4.1725363427.git.andrii.sultanov@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/ocaml: Stabilize domain_getinfo for Oxenstored | expand |
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
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@"
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(-)