Libguestfs - Jul 2023 - [libnbd PATCH 09/10] rust: async: Create an async friendly handle type

On Wed, Jul 19, 2023 at 09:09:53AM +0000, Tage Johansson
wrote:
> Create another handle type: `AsyncHandle`, which makes use of Rust's
> builtin asynchronous functions (see
> <https://doc.rust-lang.org/std/keyword.async.html>) and runs on top
of
> the Tokio runtime (see <https://docs.rs/tokio>). For every
asynchronous
> command, like `aio_connect()`, a corresponding `async` method is created
> on the handle. In this case it would be:
>     async fn connect(...) -> Result<(), ...>
> When called, it will poll the file descriptor until the command is
> complete, and then return with a result. All the synchronous
> counterparts (like `nbd_connect()`) are excluded from this handle type
> as they are unnecessary and since they might interfear with the polling
> made by the Tokio runtime. For more details about how the asynchronous
> commands are executed, please see the comments in
> rust/src/async_handle.rs.
This is an interesting (and good) approach, layering a more natural
API for Rust users on top of the "low level" API.
> ---
>  generator/Rust.ml        | 232 +++++++++++++++++++++++++++++++++++++++
>  generator/Rust.mli       |   2 +
>  generator/generator.ml   |   1 +
>  rust/Cargo.toml          |   1 +
>  rust/Makefile.am         |   1 +
>  rust/src/async_handle.rs | 222 +++++++++++++++++++++++++++++++++++++
>  rust/src/lib.rs          |   4 +
>  7 files changed, 463 insertions(+)
>  create mode 100644 rust/src/async_handle.rs
> 
> diff --git a/generator/Rust.ml b/generator/Rust.ml
> index 96095a9..1048831 100644
> --- a/generator/Rust.ml
> +++ b/generator/Rust.ml
> @@ -601,3 +601,235 @@ let generate_rust_bindings () >    pr "impl
Handle {\n";
>    List.iter print_rust_handle_method handle_calls;
>    pr "}\n\n"
> +
> +(*********************************************************)
> +(* The rest of the file conserns the asynchronous API.   *)
> +(*                                                       *)
> +(* See the comments in rust/src/async_handle.rs for more *)
> +(* information about how it works.                       *)
> +(*********************************************************)
> +
> +let excluded_handle_calls : NameSet.t > +  NameSet.of_list
> +    [
> +      "aio_get_fd";
> +      "aio_get_direction";
> +      "aio_notify_read";
> +      "aio_notify_write";
> +      "clear_debug_callback";
> +      "get_debug";
> +      "poll";
> +      "poll2";
> +      "set_debug";
> +      "set_debug_callback";
> +    ]
This is a "code smell" since all information that is specific to APIs
should (usually) go in generator/API.ml.

It's reasonable for aio_get_{fd,direction} aio_notify_{read,write}
since those are very special APIs, but I don't understand why the poll
and debug functions are listed here.  What's the real meaning of this
list of functions, ie. why can each one not be included in the tokio
bindings?
> +
> +(* A mapping with names as keys. *)
> +module NameMap = Map.Make (String)
> +
> +(* Strip "aio_" from the beginning of a string. *)
> +let strip_aio name : string > +  if String.starts_with
~prefix:"aio_" name then
> +    String.sub name 4 (String.length name - 4)
> +  else failwith (sprintf "Asynchronous call %s must begin with
aio_" name)
> +
> +(* A map with all asynchronous handle calls. The keys are names with
"aio_"
> +   stripped, the values are a tuple with the actual name (with
"aio_"), the
> +   [call] and the [async_kind]. *)
> +let async_handle_calls : ((string * call) * async_kind) NameMap.t > + 
handle_calls
> +  |> List.filter (fun (n, _) -> not (NameSet.mem n
excluded_handle_calls))
> +  |> List.filter_map (fun (name, call) ->
> +         call.async_kind
> +         |> Option.map (fun async_kind ->
> +                (strip_aio name, ((name, call), async_kind))))
> +  |> List.to_seq |> NameMap.of_seq
> +
> +(* A mapping with all synchronous (not asynchronous) handle calls.
Excluded
> +   are also all synchronous calls that has an asynchronous counterpart. So
if
> +   "foo" is the name of a handle call and an asynchronous call
"aio_foo"
> +   exists, then "foo" will not b in this map. *)
> +let sync_handle_calls : call NameMap.t > +  handle_calls
> +  |> List.filter (fun (n, _) -> not (NameSet.mem n
excluded_handle_calls))
> +  |> List.filter (fun (name, _) ->
> +         (not (NameMap.mem name async_handle_calls))
> +         && not
> +              (String.starts_with ~prefix:"aio_" name
> +              && NameMap.mem (strip_aio name) async_handle_calls))
> +  |> List.to_seq |> NameMap.of_seq
> +
> +(* Get the Rust type for an argument in the asynchronous API. Like
> +   [rust_arg_type] but no static lifetime on some closures and buffers. *)
> +let rust_async_arg_type : arg -> string = function
> +  | Closure { cbargs; cbkind } ->
> +      let lifetime > +        match cbkind with
> +        | CBOnceCommand | CBManyCommand -> None
> +        | CBManyHandle -> Some "'static"
> +      in
> +      "impl " ^ rust_closure_trait ~lifetime cbargs cbkind
> +  | BytesPersistIn _ -> "&[u8]"
> +  | BytesPersistOut _ -> "&mut [u8]"
> +  | x -> rust_arg_type x
> +
> +(* Get the Rust type for an optional argument in the asynchronous API.
Like
> +   [rust_optarg_type] but no static lifetime on some closures. *)
> +let rust_async_optarg_type : optarg -> string = function
> +  | OClosure x -> sprintf "Option<%s>"
(rust_async_arg_type (Closure x))
> +  | x -> rust_optarg_type x
> +
> +(* A string of the argument list for a method on the handle, with both
> +   mandotory and optional arguments. *)
> +let rust_async_handle_call_args { args; optargs } : string > +  let
rust_args_names > +    List.map rust_arg_name args @ List.map
rust_optarg_name optargs
> +  and rust_args_types > +    List.map rust_async_arg_type args
> +    @ List.map rust_async_optarg_type optargs
> +  in
> +  String.concat ", "
> +    (List.map2
> +       (fun name typ -> name ^ ": " ^ typ)
Often sprintf is nicer, and in this case it really works well:

  (List.map2 (sprintf "%s: %s") rust_args_names rust_args_types)

OCaml will actually generate identical code for this as for your
version, since sprintf is handled as a kind of macro.

You will need "open Printf" at the top of the file, if it is not there
already.
> +(* Print the Rust function for a not asynchronous handle call. *)
> +let print_rust_sync_handle_call (name : string) (call : call) > + 
print_rust_handle_call_comment call;
> +  pr "pub fn %s(&self, %s) -> %s\n" name
> +    (rust_async_handle_call_args call)
> +    (rust_ret_type call);
> +  print_ffi_call name "self.data.handle.handle" call;
> +  pr "\n"
> +
> +(* Print the Rust function for an asynchronous handle call with a
completion
> +   callback. (Note that "callback" might be abbreviated with
"cb" in the
> +   following code. *)
> +let print_rust_async_handle_call_with_completion_cb name (aio_name, call)
> +  (* An array of all optional arguments. Useful because we need to deel
with
> +     the index of the completion callback. *)
> +  let optargs = Array.of_list call.optargs in
> +  (* The index of the completion callback in [optargs] *)
> +  let completion_cb_index > +    Array.find_map
> +      (fun (i, optarg) ->
> +        match optarg with
> +        | OClosure { cbname } ->
> +            if cbname = "completion" then Some i else None
> +        | _ -> None)
> +      (Array.mapi (fun x y -> (x, y)) optargs)
> +  in
> +  let completion_cb_index > +    match completion_cb_index with
> +    | Some x -> x
> +    | None ->
> +        failwith
> +          (sprintf
> +             "The handle call %s is claimed to have a completion
callback \
> +              among its optional arguments by the async_kind field, but so
\
> +              does not seem to be the case."
> +             aio_name)
Conversely, here you an just use "failwithf" (failwith + sprintf)!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 7/19/2023 4:47 PM, Richard W.M. Jones wrote:
> On Wed, Jul 19, 2023 at 09:09:53AM +0000, Tage Johansson wrote:
>> Create another handle type: `AsyncHandle`, which makes use of
Rust's
>> builtin asynchronous functions (see
>> <https://doc.rust-lang.org/std/keyword.async.html>) and runs on
top of
>> the Tokio runtime (see<https://docs.rs/tokio>). For every
asynchronous
>> command, like `aio_connect()`, a corresponding `async` method is
created
>> on the handle. In this case it would be:
>>      async fn connect(...) -> Result<(), ...>
>> When called, it will poll the file descriptor until the command is
>> complete, and then return with a result. All the synchronous
>> counterparts (like `nbd_connect()`) are excluded from this handle type
>> as they are unnecessary and since they might interfear with the polling
>> made by the Tokio runtime. For more details about how the asynchronous
>> commands are executed, please see the comments in
>> rust/src/async_handle.rs.
> This is an interesting (and good) approach, layering a more natural
> API for Rust users on top of the "low level" API.
>
>> ---
>>   generator/Rust.ml        | 232
+++++++++++++++++++++++++++++++++++++++
>>   generator/Rust.mli       |   2 +
>>   generator/generator.ml   |   1 +
>>   rust/Cargo.toml          |   1 +
>>   rust/Makefile.am         |   1 +
>>   rust/src/async_handle.rs | 222 +++++++++++++++++++++++++++++++++++++
>>   rust/src/lib.rs          |   4 +
>>   7 files changed, 463 insertions(+)
>>   create mode 100644 rust/src/async_handle.rs
>>
>> diff --git a/generator/Rust.ml b/generator/Rust.ml
>> index 96095a9..1048831 100644
>> --- a/generator/Rust.ml
>> +++ b/generator/Rust.ml
>> @@ -601,3 +601,235 @@ let generate_rust_bindings () >>     pr
"impl Handle {\n";
>>     List.iter print_rust_handle_method handle_calls;
>>     pr "}\n\n"
>> +
>> +(*********************************************************)
>> +(* The rest of the file conserns the asynchronous API.   *)
>> +(*                                                       *)
>> +(* See the comments in rust/src/async_handle.rs for more *)
>> +(* information about how it works.                       *)
>> +(*********************************************************)
>> +
>> +let excluded_handle_calls : NameSet.t >> +  NameSet.of_list
>> +    [
>> +      "aio_get_fd";
>> +      "aio_get_direction";
>> +      "aio_notify_read";
>> +      "aio_notify_write";
>> +      "clear_debug_callback";
>> +      "get_debug";
>> +      "poll";
>> +      "poll2";
>> +      "set_debug";
>> +      "set_debug_callback";
>> +    ]
> This is a "code smell" since all information that is specific to
APIs
> should (usually) go in generator/API.ml.
>
> It's reasonable for aio_get_{fd,direction} aio_notify_{read,write}
> since those are very special APIs, but I don't understand why the poll
> and debug functions are listed here.  What's the real meaning of this
> list of functions, ie. why can each one not be included in the tokio
> bindings?

The debug functions are used for setting up logging with the log crate 
<https://docs.rs/log> in rust/src/handle.rs:

```Rust

impl Handle {
 ??? pub fn new() -> Result<Self> {
 ??????? let handle = unsafe { sys::nbd_create() };
 ??????? 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")]
 ??????????? {
 ??????????????? nbd.set_debug_callback(|func_name, msg| {
 ??????????????????? log::debug!(
 ??????????????????????? target: func_name.to_string_lossy().as_ref(),
 ??????????????????????? "{}",
 ??????????????????????? msg.to_string_lossy()
 ??????????????????? );
 ??????????????????? 0
 ??????????????? })?;
 ??????????????? nbd.set_debug(true)?;
 ??????????? }
 ??????????? Ok(nbd)
 ??????? }
 ??? }

```


So if the user would set another debug callback this functionality would 
stop working. And since the documentation is automaticly generated for 
`nbd_set_debug_callback()`, the user might not expect that logging just 
stops working because they set a debug callback. But I guess that I can 
add a note about that to the documentation of the `Handle` struct and 
include the debug callbacks, if you want?


Regarding the other methods, it's a bit more complicated. The problem is 
that calling any `aio_notify_*` method might cause any ongoing command 
to finish. For example, the `connect()` method on `AsyncHandle` will not 
return until `aio_is_connecting()` returns false. In order not to have 
to check that in a busy loop, the function is woken up only when the 
polling task makes a call to any `aio_notify_*` function. So whenever 
the polling task calls `aio_notify_*`, it notifies the waiting command 
that the handle might has changed state. But if the user calls 
`aio_notify_*` (directly or indirectly) while `aio_connect` is in 
flight, it might happen that the connection completes without the call 
to `connect()` gets notified about that.


This means that any function that calls any `aio_notify_*` function 
should not be implemented on `AsyncHandle`. A better approach than the 
current would probably be to add a field to the `call`-structure in 
API.ml. Something like `notifies_fd : bool` (sure someone can come up 
with a better name). This would be set to true for any function that 
calls `aio_notify_*`, including `aio_notify_read()`, `poll()`, and all 
synchronous commands like `nbd_connect()`. What do you think about that?


I have written some more explonations about how the asynchronous handle 
works in rust/src/async_handle.rs, but it is somewhat complicated so 
please ask if there are more unclear things.


>> +
>> +(* A mapping with names as keys. *)
>> +module NameMap = Map.Make (String)
>> +
>> +(* Strip "aio_" from the beginning of a string. *)
>> +let strip_aio name : string >> +  if String.starts_with
~prefix:"aio_" name then
>> +    String.sub name 4 (String.length name - 4)
>> +  else failwith (sprintf "Asynchronous call %s must begin with
aio_" name)
>> +
>> +(* A map with all asynchronous handle calls. The keys are names with
"aio_"
>> +   stripped, the values are a tuple with the actual name (with
"aio_"), the
>> +   [call] and the [async_kind]. *)
>> +let async_handle_calls : ((string * call) * async_kind) NameMap.t
>> +  handle_calls
>> +  |> List.filter (fun (n, _) -> not (NameSet.mem n
excluded_handle_calls))
>> +  |> List.filter_map (fun (name, call) ->
>> +         call.async_kind
>> +         |> Option.map (fun async_kind ->
>> +                (strip_aio name, ((name, call), async_kind))))
>> +  |> List.to_seq |> NameMap.of_seq
>> +
>> +(* A mapping with all synchronous (not asynchronous) handle calls.
Excluded
>> +   are also all synchronous calls that has an asynchronous
counterpart. So if
>> +   "foo" is the name of a handle call and an asynchronous
call "aio_foo"
>> +   exists, then "foo" will not b in this map. *)
>> +let sync_handle_calls : call NameMap.t >> +  handle_calls
>> +  |> List.filter (fun (n, _) -> not (NameSet.mem n
excluded_handle_calls))
>> +  |> List.filter (fun (name, _) ->
>> +         (not (NameMap.mem name async_handle_calls))
>> +         && not
>> +              (String.starts_with ~prefix:"aio_" name
>> +              && NameMap.mem (strip_aio name)
async_handle_calls))
>> +  |> List.to_seq |> NameMap.of_seq
>> +
>> +(* Get the Rust type for an argument in the asynchronous API. Like
>> +   [rust_arg_type] but no static lifetime on some closures and
buffers. *)
>> +let rust_async_arg_type : arg -> string = function
>> +  | Closure { cbargs; cbkind } ->
>> +      let lifetime >> +        match cbkind with
>> +        | CBOnceCommand | CBManyCommand -> None
>> +        | CBManyHandle -> Some "'static"
>> +      in
>> +      "impl " ^ rust_closure_trait ~lifetime cbargs cbkind
>> +  | BytesPersistIn _ -> "&[u8]"
>> +  | BytesPersistOut _ -> "&mut [u8]"
>> +  | x -> rust_arg_type x
>> +
>> +(* Get the Rust type for an optional argument in the asynchronous API.
Like
>> +   [rust_optarg_type] but no static lifetime on some closures. *)
>> +let rust_async_optarg_type : optarg -> string = function
>> +  | OClosure x -> sprintf "Option<%s>"
(rust_async_arg_type (Closure x))
>> +  | x -> rust_optarg_type x
>> +
>> +(* A string of the argument list for a method on the handle, with both
>> +   mandotory and optional arguments. *)
>> +let rust_async_handle_call_args { args; optargs } : string >> + 
let rust_args_names >> +    List.map rust_arg_name args @ List.map
rust_optarg_name optargs
>> +  and rust_args_types >> +    List.map rust_async_arg_type args
>> +    @ List.map rust_async_optarg_type optargs
>> +  in
>> +  String.concat ", "
>> +    (List.map2
>> +       (fun name typ -> name ^ ": " ^ typ)
> Often sprintf is nicer, and in this case it really works well:
>
>    (List.map2 (sprintf "%s: %s") rust_args_names rust_args_types)
>
> OCaml will actually generate identical code for this as for your
> version, since sprintf is handled as a kind of macro.
>
> You will need "open Printf" at the top of the file, if it is not
there
> already.
I will fix that.


Best regards,

Tage

>> +(* Print the Rust function for a not asynchronous handle call. *)
>> +let print_rust_sync_handle_call (name : string) (call : call) >>
+  print_rust_handle_call_comment call;
>> +  pr "pub fn %s(&self, %s) -> %s\n" name
>> +    (rust_async_handle_call_args call)
>> +    (rust_ret_type call);
>> +  print_ffi_call name "self.data.handle.handle" call;
>> +  pr "\n"
>> +
>> +(* Print the Rust function for an asynchronous handle call with a
completion
>> +   callback. (Note that "callback" might be abbreviated with
"cb" in the
>> +   following code. *)
>> +let print_rust_async_handle_call_with_completion_cb name (aio_name,
call) >> +  (* An array of all optional arguments. Useful because we need
to deel with
>> +     the index of the completion callback. *)
>> +  let optargs = Array.of_list call.optargs in
>> +  (* The index of the completion callback in [optargs] *)
>> +  let completion_cb_index >> +    Array.find_map
>> +      (fun (i, optarg) ->
>> +        match optarg with
>> +        | OClosure { cbname } ->
>> +            if cbname = "completion" then Some i else None
>> +        | _ -> None)
>> +      (Array.mapi (fun x y -> (x, y)) optargs)
>> +  in
>> +  let completion_cb_index >> +    match completion_cb_index with
>> +    | Some x -> x
>> +    | None ->
>> +        failwith
>> +          (sprintf
>> +             "The handle call %s is claimed to have a completion
callback \
>> +              among its optional arguments by the async_kind field,
but so \
>> +              does not seem to be the case."
>> +             aio_name)
> Conversely, here you an just use "failwithf" (failwith +
sprintf)!
>
> Rich.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20230721/5b6191ac/attachment.htm>