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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Reply via email to