[Libguestfs] [libnbd PATCH 04/10] generator: Add information about the lifetime of closures

Mon Jul 24 20:04:15 UTC 2023

On Mon, Jul 24, 2023 at 03:22:56PM -0400, Stefan Hajnoczi wrote:
> On Fri, Jul 21, 2023 at 11:37:09AM +0100, Richard W.M. Jones wrote:
> > On Fri, Jul 21, 2023 at 10:13:02AM +0000, Tage Johansson wrote:
> > > 
> > > On 7/19/2023 4:35 PM, Richard W.M. Jones wrote:
> > > >On Wed, Jul 19, 2023 at 09:09:48AM +0000, Tage Johansson wrote:
> > > >>Add a new field `cbkind` to the `closure` type in generator/API.ml*.
> > > >>It tells how many times the closure may be invoked and for how long time
> > > >>it might be used. More specifically, it can take any of these values:
> > > >>- `CBOnceCommand`: The closure may only be called once and shall not
> > > >>   be called after the command is retired.
> > > >>- `CBManyCommand`: The closure may be called any number of times but
> > > >>   not after the command is retired.
> > > >>- `CBManyHandle`: The closure may be called any number of times before
> > > >>   the handle is destructed.
> > > >>This information is needed in the Rust bindings for:
> > > >>a) Knowing if the closure trait should be `FnMut` or `FnOnce`
> > > >>    (see <https://doc.rust-lang.org/std/ops/trait.FnOnce.html>).
> > > >>b) Knowing for what lifetime the closure should be valid. A closure that
> > > >>    may be called after the function invokation has returned must live
> > > >>    for the `'static` lifetime. But static closures are inconveniant for
> > > >>    the user since they can't effectively borrow any local data. So it is
> > > >>    good if this restriction is relaxed when it is not needed.
> > > >>---
> > > >>  generator/API.ml  | 11 +++++++++++
> > > >>  generator/API.mli | 13 +++++++++++++
> > > >>  2 files changed, 24 insertions(+)
> > > >>
> > > >>diff --git a/generator/API.ml b/generator/API.ml
> > > >>index f90a6fa..086b2f9 100644
> > > >>--- a/generator/API.ml
> > > >>+++ b/generator/API.ml
> > > >>@@ -77,6 +77,7 @@ and ret =
> > > >>  and closure = {
> > > >>    cbname : string;
> > > >>    cbargs : cbarg list;
> > > >>+  cbkind : cbkind;
> > > >I'm dubious about the premise of this patch, but let's at least call
> > > >it "cblifetime" since that's what it is expressing.
> > > 
> > > 
> > > The difference in code for the user might be something like the following:
> > > 
> > > With only static lifetimes, a call to `opt_list` might look like this:
> > > 
> > > ```Rust
> > > 
> > > use std::sync::{Arc, Mutex};        // Collect all exports in this list.
> > > 
> > > 
> > > // Collect all exports in this list.
> > > let exports = Arc::new(Mutex::new(Vec::new()));
> > > let exports_clone = exports.clone();
> > > let count = nbd.opt_list(move |name, _| {
> > >     exports_clone.lock().unwrap().push(name.to_owned());
> > >     0
> > > })?;
> > > let exports = Arc::into_inner(exports).unwrap().into_inner().unwrap();
> > > assert_eq!(export.as_c_str(), expected);
> > > ```
> > > 
> > > 
> > > And with custom lifetimes:
> > > 
> > > ```Rust
> > > 
> > > // Collect all exports in this list.
> > > let mut exports = Vec::new();
> > > let count = nbd.opt_list(|name, _| {
> > >     exports.push(name.to_owned());
> > >     0
> > > })?;
> > > assert_eq!(exports.len(), count as usize);
> > > ```
> > > 
> > > 
> > > Not only is the latter shorter and easier to read, it is also more
> > > efficient. But it is not strictly necessary, and I can remove it if
> > > you want.
> > 
> > Stefan - any thoughts on this?
> > 
> > From my point of view the issue is that attempting to categorize
> > libnbd callbacks according to their lifetime complicates the API
> > description and might shut down (or make complicated) future more
> > complex patterns of callback use.
> > 
> > The performance issue is not very critical given that we're already
> > going through a C library to Rust layer.  A reference count doesn't
> > seem like a big deal to me.
> 
> If the generated Rust API involves closures then dealing with Fn,
> FnOnce, FnMut is necessary.
> 
> It may be more natural to use the Iterator trait or other Rust features
> instead of closures in some cases. Doing so might allow you to avoid
> dealing with FnOnce, Fn, and FnMut while also making the Rust API nicer.
> 
> Are the generated API docs available somewhere so I can get an
> understanding of the Rust API?

I don't think we're publishing those for Rust yet.  Tage ..?

The C API docs are published.  An example might be the
nbd_block_status API where (because we get a potentially long list of
extents from the server, and we get them asynchronously) we call back
to a function that the caller provides:

  https://libguestfs.org/nbd_block_status.3.html

  https://gitlab.com/nbdkit/libnbd/-/blob/5c2fc3cc7e14146d000b65b191e70d9a0585a395/info/map.c#L73
  https://gitlab.com/nbdkit/libnbd/-/blob/5c2fc3cc7e14146d000b65b191e70d9a0585a395/info/map.c#L108

This is how OCaml binds it ('fun meta _ entries err ->' is the
callback closure):

  https://gitlab.com/nbdkit/libnbd/-/blob/master/ocaml/examples/extents.ml#L17

Not all callbacks work like nbd_block_status though.  It is possible
to register a callback which can be called much later, after the
function that registered it has returned.

In C each callback has an associated '.free' function.  This is
guaranteed to be called exactly once, after the callback itself will
no longer be called.  It can be used to free the callback data; or for
GC'd bindings like OCaml, to dereference the global root so it will be
garbage collected.

General discussion:

  https://libguestfs.org/libnbd.3.html#CALLBACKS

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top