Re: [Libguestfs] [virt-v2v PATCH 1/3] lib/nbdkit: add the "Nbdkit.with_connect_unix" helper function

On 12/08/21 16:18, Richard W.M. Jones wrote: > On Wed, Dec 08, 2021 at 01:20:48PM +0100, Laszlo Ersek wrote: >> Connecting to an NBD server temporarily, for a "one-shot" operation, is >> quite similar to "Std_utils.with_open_in" and "Std_utils.with_open_out", >> as there are cleanup operations regardless of whether the "one-shot" >> operation completes successfully or throws an exception. >> >> Introduce the "Nbdkit.with_connect_unix" function, which takes a Unix >> domain socket pathname, a list of metadata contexts to request from the >> NBD server, and calls a function with the live NBD server connection. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> lib/Makefile.am | 2 +- >> lib/nbdkit.mli | 10 ++++++++++ >> lib/nbdkit.ml | 12 ++++++++++++ >> 3 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/lib/Makefile.am b/lib/Makefile.am >> index c274b9ecf6c7..1fab25b326f5 100644 >> --- a/lib/Makefile.am >> +++ b/lib/Makefile.am >> @@ -80,7 +80,7 @@ BOBJECTS = config.cmo $(SOURCES_ML:.ml=.cmo) >> XOBJECTS = $(BOBJECTS:.cmo=.cmx) >> >> OCAMLPACKAGES = \ >> - -package str,unix \ >> + -package str,unix,nbd \ >> -I $(builddir) \ >> -I $(top_builddir)/common/mlgettext \ >> -I $(top_builddir)/common/mlpcre \ >> diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli >> index ae19295186ed..825755e61dbe 100644 >> --- a/lib/nbdkit.mli >> +++ b/lib/nbdkit.mli >> @@ -107,3 +107,13 @@ val run_unix : ?socket:string -> cmd -> string * int >> The --exit-with-parent, --foreground, --pidfile, --newstyle and >> --unix flags are added automatically. Other flags are set as >> in the {!cmd} struct. *) >> + >> +val with_connect_unix : socket:string -> >> + meta_contexts:string list -> >> + f:(NBD.t -> 'a) -> >> + 'a >> +(** [with_connect_unix socket meta_contexts f] calls function [f] with the NBD >> + server at Unix domain socket [socket] connected, and the metadata contexts >> + in [meta_contexts] requested (each of which is not necessarily supported by >> + the server though). The connection is torn down either on normal return or >> + if the function [f] throws an exception. *) > > Interesting choice of module for this. nbdkit is a server. I would > have put it into lib/utils.mli or created a new module. Yes, I was certainly lost as to what module to pick :) I found "run_unix" in this one, and thought that a wrapper for "connect_unix" (from the OCaml binding of libnbd) belonged here. I can add it to "utils" if you think that's a good fit. (I feel that creating a new module just for this is overkill, but I could be wrong.)

> The function itself looks fine although personally I might have > abstracted the meta_context function to be a "do anything between > creation and connection" (let's call that "preparation"). What do you > think about: > > val with_connect_unix : socket:string -> > ?prepare:(NBD.t -> unit) -> > f:(NBD.t -> 'a) -> > 'a > > I think you would have to replace the List.iter line: > > ... >> + ~f:(fun () -> > > (match prepare with Some pf -> pf nbd | None -> ()); > >> + NBD.connect_unix nbd socket; >> + protect >> + ~f:(fun () -> f nbd) >> + ~finally:(fun () -> NBD.shutdown nbd) >> + ) >> + ~finally:(fun () -> NBD.close nbd) > > It would be called like this: > > let prepare nbd = NBD.add_meta_context nbd "base:allocation" in > with_connect_unix socket ~prepare ( > fun () -> > (* the code *) > ) > > Hmm, maybe this is getting more complicated :-( Well my concern is the following; let me paste the full function again: let with_connect_unix ~socket ~meta_contexts ~f = let nbd = NBD.create () in protect ~f:(fun () -> List.iter (NBD.add_meta_context nbd) meta_contexts; NBD.connect_unix nbd socket; protect ~f:(fun () -> f nbd) ~finally:(fun () -> NBD.shutdown nbd) ) ~finally:(fun () -> NBD.close nbd) "NBD.close", which mirrors "NBD.create", suffices for undoing "NBD.add_meta_context" as well. But, if we turn "NBD.add_meta_context" into a generic "do anything between NBD.create and NBD.connect_unix", I don't know if "NBD.close" can still satisfy the teardown role. In that case, we might have to accept another "rollback" function, and *that* I do find too complicated. FWIW, the current code does make a similar assumption about "f", and "NBD.shutdown". However, I thought that that was OK (and didn't need spelling out): namely, once "NBD.connect_unix" completes, any valid operation on the connection is considered... well, valid (or "expected"), and "NBD.shutdown" is considered appropriate to finish *any* such sequence of operations. I don't have the same confidence in a generic "prepare" being undone by just NBD.close.