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

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.

From my point of view the issue is that attempting to categorize

You are correct though that `cbkind` is not a particularly good name. `cblifetime` might be better, but actually it is describing two things: Both the lifetime of the closure and how many times the closure might be called. Perhaps it might be better to split it into two fields: `cblifetime` and `cbcount`? >> } >> and cbarg = >> | CBArrayAndLen of arg * string >>@@ -87,6 +88,10 @@ and cbarg = >> | CBString of string >> | CBUInt of string >> | CBUInt64 of string >>+and cbkind = >>+| CBOnceCommand >>+| CBManyCommand >>+| CBManyHandle >> and enum = { >> enum_prefix : string; >> enums : (string * int) list >>@@ -141,20 +146,24 @@ the handle from the NBD protocol handshake." >> (* Closures. *) >> let chunk_closure = { >> cbname = "chunk"; >>+ cbkind = CBManyCommand; >> cbargs = [ CBBytesIn ("subbuf", "count"); >> CBUInt64 "offset"; CBUInt "status"; >> CBMutable (Int "error") ] >> } >> let completion_closure = { >> cbname = "completion"; >>+ cbkind = CBOnceCommand; >> cbargs = [ CBMutable (Int "error") ] >> } >> let debug_closure = { >> cbname = "debug"; >>+ cbkind = CBManyHandle; >> cbargs = [ CBString "context"; CBString "msg" ] >> } >> let extent_closure = { >> cbname = "extent"; >>+ cbkind = CBManyCommand; >> cbargs = [ CBString "metacontext"; >> CBUInt64 "offset"; >> CBArrayAndLen (UInt32 "entries", >>@@ -163,10 +172,12 @@ let extent_closure = { >> } >> let list_closure = { >> cbname = "list"; >>+ cbkind = CBManyCommand; >> cbargs = [ CBString "name"; CBString "description" ] >> } >> let context_closure = { >> cbname = "context"; >>+ cbkind = CBManyCommand; >> cbargs = [ CBString "name" ] >> } >> let all_closures = [ chunk_closure; completion_closure; >>diff --git a/generator/API.mli b/generator/API.mli >>index 361132d..73dc40a 100644 >>--- a/generator/API.mli >>+++ b/generator/API.mli >>@@ -94,6 +94,10 @@ and ret = >> and closure = { >> cbname : string; (** name of callback function *) >> cbargs : cbarg list; (** all closures return int for now *) >>+ (** Wether the closure should be used once or many times and wether it might >wether -> whether > >Rich. > >>+ be used only until the command is completed or until the handle is >>+ destructed. *) >>+ cbkind : cbkind; >> } >> and cbarg = >> | CBArrayAndLen of arg * string (** array + number of entries *) >>@@ -104,6 +108,15 @@ and cbarg = >> | CBString of string (** like String *) >> | CBUInt of string (** like UInt *) >> | CBUInt64 of string (** like UInt64 *) >>+and cbkind = >>+| CBOnceCommand (** The closure will be used 0 or 1 time if the aio_* call >>+ returned a error and exactly once if the call succeeded. >>+ The closure may only be called before the command >>+ is retired. (E.g., completion callback.) *) >>+| CBManyCommand (** The closure may be used any number of times but only >>+ before the command is retired. (E.g., list callback.) *) >>+| CBManyHandle (** The closure may be used any number of times for as long >>+ as the handle exists. (E.g., debug callback.) *) >> and enum = { >> enum_prefix : string; (** prefix of each enum variant *) >> enums : (string * int) list (** enum names and their values in C *)