Re: [PATCH v2 3/3] tools/oxenstored: Use the plugin for Xenctrl.domain_getinfo

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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