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

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