Richard W.M. Jones
2023-Jul-21 10:37 UTC
[Libguestfs] [libnbd PATCH 04/10] generator: Add information about the lifetime of closures
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 categorizelibnbd 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.> 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 *)Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Stefan Hajnoczi
2023-Jul-24 19:22 UTC
[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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://listman.redhat.com/archives/libguestfs/attachments/20230724/b1709827/attachment.sig>