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

On 7/25/2023 8:03 PM, Stefan Hajnoczi wrote: > On Mon, Jul 24, 2023 at 09:04:15PM +0100, Richard W.M. Jones wrote: > > 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/5c2fc3cc7e14146d000b65b191e70d9a0... > > https://gitlab.com/nbdkit/libnbd/-/blob/5c2fc3cc7e14146d000b65b191e70d9a0... > > > > 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 > In Rust you have the choice between function pointers > (https://doc.rust-lang.org/std/primitive.fn.html) and the FnOnce, Fn, > FnMut traits (https://doc.rust-lang.org/std/ops/trait.FnOnce.html). > Function pointers are similar to C and do not support closures. For > closures you need the FnOnce, Fn, or FnMut traits. > > I think Tage's approach fits well if you want closures. If you don't > need closures then you can simplify by using function pointers. > > If you're willing to diverge from the C API, then the Rust API could > favor other approaches instead of passing callbacks (e.g. use the > Iterator trait instead of closures for extracting items from > collections). That could make Rust application code nicer. I guess in > some cases callbacks are really the right approach though. > > Stefan The problem with iterators is that we would need a so called "lending iterator" <https://docs.rs/lending-iterator/latest/lending_iterator/> which is unfortunately still quite hard to use in Rust. So I think that a closure suffices here. The hard question isn't really though if `FnOnce` or `FnMut` should be used, but what the lifetime constraint of those closures would be. If all of them would have to be `'static`, or if some can be of a more convenient lifetime. I try to implement this with the `cblifetime` attribute (see [libnbd PATCH v3 03/10].