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

[Richard W.M. Jones]

On Thu, Dec 09, 2021 at 11:47:12AM +0100, Laszlo Ersek wrote:
> 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 at redhat.com>
> >> ---
> >>  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.)

Go with Utils.  It's a dumping ground for random functions used in
virt-v2v.

> > 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.

Sure, let's go with the function as originally written, I think I was
trying to over-abstract things.  We can always change it later if we
need to.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW