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

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.

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 :-(

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

Thanks
Laszlo