[Libguestfs] [libnbd PATCH v4 04/11] rust: Use more specific closure traits
Tage Johansson
tage.j.lists at posteo.net
Thu Aug 3 15:27:51 UTC 2023
On 8/2/2023 6:32 PM, Richard W.M. Jones wrote:
> On Wed, Aug 02, 2023 at 12:40:49PM +0000, Tage Johansson wrote:
>> For closures with `cb,count = CBOnce`, `FnOnce` will be used instead of
> I think there's an extra ',' here.
>
> Previous comments about use of markdown apply too.
>
>> `FnMut`. Moreover, closures in synchronous commands with
>> `cblifetime = CBCommand` will not need to live for the static lifetime.
>> See [here](https://doc.rust-lang.org/std/ops/trait.FnOnce.html) for more
>> information about the advantages of using `FnOnce` when possible.
>> ---
>> generator/Rust.ml | 44 ++++++++++++++++++++++++++++----------------
>> rust/src/handle.rs | 2 ++
>> rust/src/types.rs | 2 ++
>> 3 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/generator/Rust.ml b/generator/Rust.ml
>> index 3117980..d3225eb 100644
>> --- a/generator/Rust.ml
>> +++ b/generator/Rust.ml
>> @@ -111,7 +111,7 @@ let rust_cbarg_name : cbarg -> string = function
>> | CBArrayAndLen (arg, _) | CBMutable arg -> rust_arg_name arg
>>
>> (* Get the Rust type for an argument. *)
>> -let rec rust_arg_type : arg -> string = function
>> +let rec rust_arg_type ?(async_kind = None) : arg -> string = function
>> | Bool _ -> "bool"
>> | Int _ -> "c_int"
>> | UInt _ -> "c_uint"
>> @@ -131,15 +131,18 @@ let rec rust_arg_type : arg -> string = function
>> | BytesOut _ -> "&mut [u8]"
>> | BytesPersistIn _ -> "&'static [u8]"
>> | BytesPersistOut _ -> "&'static mut [u8]"
>> - | Closure { cbargs } -> "impl " ^ rust_closure_trait cbargs
>> + | Closure { cbargs; cbcount } -> (
>> + match async_kind with
>> + | Some _ -> "impl " ^ rust_closure_trait cbargs cbcount
>> + | None -> "impl " ^ rust_closure_trait cbargs cbcount ~lifetime:None)
>>
>> (* Get the Rust closure trait for a callback, That is `Fn*(...) -> ...)`. *)
>> -and rust_closure_trait ?(lifetime = Some "'static") cbargs : string =
>> +and rust_closure_trait ?(lifetime = Some "'static") cbargs cbcount : string =
>> let rust_cbargs = String.concat ", " (List.map rust_cbarg_type cbargs)
>> - and lifetime_constraint =
>> - match lifetime with None -> "" | Some x -> " + " ^ x
>> - in
>> - "FnMut(" ^ rust_cbargs ^ ") -> c_int + Send + Sync" ^ lifetime_constraint
>> + and closure_type =
>> + match cbcount with CBOnce -> "FnOnce" | CBMany -> "FnMut"
>> + and lifetime = match lifetime with None -> "" | Some x -> " + " ^ x in
>> + sprintf "%s(%s) -> c_int + Send + Sync%s" closure_type rust_cbargs lifetime
>>
>> (* Get the Rust type for a callback argument. *)
>> and rust_cbarg_type : cbarg -> string = function
>> @@ -153,8 +156,8 @@ and rust_cbarg_type : cbarg -> string = function
>> | CBMutable arg -> "&mut " ^ rust_arg_type arg
>>
>> (* Get the type of a rust optional argument. *)
>> -let rust_optarg_type : optarg -> string = function
>> - | OClosure x -> sprintf "Option<%s>" (rust_arg_type (Closure x))
>> +let rust_optarg_type ?(async_kind = None) : optarg -> string = function
>> + | OClosure x -> sprintf "Option<%s>" (rust_arg_type (Closure x) ~async_kind)
>> | OFlags (name, flags, _) ->
>> sprintf "Option<%s>" (rust_arg_type (Flags (name, flags)))
>>
>> @@ -419,8 +422,8 @@ let ffi_ret_to_rust (call : call) =
>> closure data, and a free function for the closure data. This struct is what
>> will be sent to a C function taking the closure as an argument. In fact,
>> the struct itself is generated by rust-bindgen. *)
>> -let print_rust_closure_to_raw_fn ({ cbname; cbargs } : closure) =
>> - let closure_trait = rust_closure_trait cbargs ~lifetime:None in
>> +let print_rust_closure_to_raw_fn ({ cbname; cbargs; cbcount } : closure) =
>> + let closure_trait = rust_closure_trait cbargs cbcount ~lifetime:None in
>> let ffi_cbargs_names = List.flatten (List.map ffi_cbarg_names cbargs) in
>> let ffi_cbargs_types = List.flatten (List.map ffi_cbarg_types cbargs) in
>> let rust_cbargs_names = List.map rust_cbarg_name cbargs in
>> @@ -435,16 +438,24 @@ let print_rust_closure_to_raw_fn ({ cbname; cbargs } : closure) =
>> (List.map2 (sprintf "%s: %s") ffi_cbargs_names ffi_cbargs_types));
>> pr " where F: %s\n" closure_trait;
>> pr " {\n";
>> - pr " let callback_ptr = data as *mut F;\n";
>> - pr " let callback = &mut *callback_ptr;\n";
>> + (match cbcount with
>> + | CBMany ->
>> + pr " let callback_ptr = data as *mut F;\n";
>> + pr " let callback = &mut *callback_ptr;\n"
>> + | CBOnce ->
>> + pr " let callback_ptr = data as *mut Option<F>;\n";
>> + pr " let callback_option: &mut Option<F> = &mut *callback_ptr;\n";
>> + pr " let callback: F = callback_option.take().unwrap();\n");
>> List.iter ffi_cbargs_to_rust cbargs;
>> pr " callback(%s)\n" (String.concat ", " rust_cbargs_names);
>> pr " }\n";
>> - pr " let callback_data = Box::into_raw(Box::new(f));\n";
>> + pr " let callback_data = Box::into_raw(Box::new(%s));\n"
>> + (match cbcount with CBMany -> "f" | CBOnce -> "Some(f)");
>> pr " sys::nbd_%s_callback {\n" cbname;
>> pr " callback: Some(call_closure::<F>),\n";
>> pr " user_data: callback_data as *mut _,\n";
>> - pr " free: Some(utils::drop_data::<F>),\n";
>> + pr " free: Some(utils::drop_data::<%s>),\n"
>> + (match cbcount with CBMany -> "F" | CBOnce -> "Option<F>");
>> pr " }\n";
>> pr "}\n";
>> pr "\n"
>> @@ -501,7 +512,8 @@ let print_rust_handle_method ((name, call) : string * call) =
>> let rust_args_names =
>> List.map rust_arg_name call.args @ List.map rust_optarg_name call.optargs
>> and rust_args_types =
>> - List.map rust_arg_type call.args @ List.map rust_optarg_type call.optargs
>> + List.map (rust_arg_type ~async_kind:call.async_kind) call.args
>> + @ List.map (rust_optarg_type ~async_kind:call.async_kind) call.optargs
>> in
>> let rust_args =
>> String.concat ", "
>> diff --git a/rust/src/handle.rs b/rust/src/handle.rs
>> index e26755c..269d1ac 100644
>> --- a/rust/src/handle.rs
>> +++ b/rust/src/handle.rs
>> @@ -31,6 +31,8 @@ impl Handle {
>> if handle.is_null() {
>> Err(unsafe { Error::Fatal(ErrorKind::get_error().into()) })
>> } else {
>> + // Set a debug callback communicating with any logging
>> + // implementation as defined by the log crate.
>> #[allow(unused_mut)]
>> let mut nbd = Handle { handle };
>> #[cfg(feature = "log")]
>> diff --git a/rust/src/types.rs b/rust/src/types.rs
>> index eb2df06..af62140 100644
>> --- a/rust/src/types.rs
>> +++ b/rust/src/types.rs
>> @@ -15,4 +15,6 @@
>> // License along with this library; if not, write to the Free Software
>> // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>
>> +/// A cookie is a 64 bit integer returned by some aio_* methods on
>> +/// [crate::Handle] used to identify a running command.
>> pub struct Cookie(pub(crate) u64);
> I still feel we're overspecifying the interface in patches 2, 3 & 4,
> just to avoid using a reference count in the Rust bindings.
>
> Rich.
The async_kind in patch 2 is necessary for the asynchronous API. The
cbkind in patch 3 is used to be able to use FnOnce when possible (I.E in
completion callbacks). The cblifetime (also in patch 3) is used to
remove the requirement on the static lifetime on most closures. Without
it the user would need an Arc
<https://doc.rust-lang.org/std/sync/struct.Arc.html> and an Mutex
<https://doc.rust-lang.org/std/sync/struct.Mutex.html> for all closures
that should collect some data.
I can remove cbkind and/or cblifetime, but without async_kind the
asynchronous API wouldn't work.
I have reordered the patches though so that the rust tests and examples
come before these generator-patches. A new patch series will come soon.
Best regards,
Tage
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20230803/e6181484/attachment.htm>
More information about the Libguestfs
mailing list