[Libguestfs] [libnbd PATCH 04/10] generator: Add information about the lifetime of closures
Stefan Hajnoczi
stefanha at redhat.com
Wed Jul 26 15:28:26 UTC 2023
On Wed, Jul 26, 2023 at 08:34:37AM +0000, Tage Johansson wrote:
>
> On 7/25/2023 8:03 PM, Stefan Hajnoczi wrote:
> > 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/5c2fc3cc7e14146d000b65b191e70d9a0585a395/info/map.c#L73
> > > https://gitlab.com/nbdkit/libnbd/-/blob/5c2fc3cc7e14146d000b65b191e70d9a0585a395/info/map.c#L108
> > >
> > > 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
>
>
> The problem with iterators is that we would need a so called "lending
> iterator" <https://docs.rs/lending-iterator/latest/lending_iterator/> which
> is unfortunately still quite hard to use in Rust. So I think that a closure
> suffices here.
>
>
> The hard question isn't really though if `FnOnce` or `FnMut` should be used,
> but what the lifetime constraint of those closures would be. If all of them
> would have to be `'static`, or if some can be of a more convenient lifetime.
> I try to implement this with the `cblifetime` attribute (see [libnbd PATCH
> v3 03/10].
The lifetimes you proposed make sense to me at first glance.
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/20230726/80b2bee7/attachment.sig>
More information about the Libguestfs
mailing list