On Tue, Aug 15, 2023 at 11:25:56AM +0000, Tage Johansson wrote: > > On 8/15/2023 11:07 AM, Richard W.M. Jones wrote: > >So what do I think about the patch series as a whole ... (in > >particular, the patches I didn't add Reviewed-by tags to). > > > >It would be much nicer IMHO if we didn't have to define callback > >lifetimes in this way, since they were not intended to be classified > >into async_kind / cblifetime / cbcount, and this might limit our > >options for new ABIs in future. > > > >I see two ways to go here: > > > >(1) (Easier for now, problems in future) Rename async_kind, cblifetime > >and cbcount as rust_async_kind, rust_cblifetime, rust_cbcount, which > >would in some sense limit the scope of getting these right to the Rust > >bindings. > > > >This defers the pain til later (maybe never, if we never added an ABI > >which didn't satisfy these constraints). > > > >(2) (Harder for now, no problems in future) Use a reference count in > >the Rust bindings, which is how the other bindings work. It makes the > >Rust bindings more awkward to use, but does more accurately express > >he actual intention of the API. > > > >Discuss ... > > > I want to emphasize the different purposes of async_kind, cblifetime > and cbcount: > > - async_kind: Tells what calls are asynchronous or not and how they > mark completion. It has nothing to do with reference counting and is > required for the asynchronous Rust bindings to work.
OK, fair point. So what I said doesn't apply to this one. > - cblifetime: Tells for how long a closure might be used. Without this > the user would need to use a reference counter (Arc) and a Mutex. The > API would be a bit more verbose/awkward to use, but it would > definitely be possible. > > - cbcount: Tells for how many times the closure might be called. Without > this, all closures would be FnMut, including completion callbacks. > I don't think it would hurt too much to remove this. > So unless someone comes up with a better solution, I think we have > to keep the async_kind field. Although it would make sense to rename > it to rust_async_kind. No, let's keep that one as "async_kind". I just misunderstood it in my first review. > Personally I think the other two makes sense as well, but they can > definitely be removed if that's what the community wants. If it's possible, I'm very much in favour of using a ref count / FnMut, in order to remove these fields. Eric, any final thoughts? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
