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

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