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