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

On 7/24/2023 9:22 PM, 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? > > Thanks, > Stefan The make script generates the docs. After running make the docs is availlable from rust/target/doc/libnbd/index.html. Just remember that you currently need LLVM installed for the bindings to build.