Tage Johansson
2023-Jul-21 09:58 UTC
[Libguestfs] [libnbd PATCH 09/10] rust: async: Create an async friendly handle type
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>
Richard W.M. Jones
2023-Jul-21 10:05 UTC
[Libguestfs] [libnbd PATCH 09/10] rust: async: Create an async friendly handle type
On Fri, Jul 21, 2023 at 09:58:52AM +0000, Tage Johansson wrote:> > 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 in rust/ > src/handle.rs:Interesting .. Is this used for all libnbd Rust handles?> ```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?I'm a bit unclear if the debugging is necessary or not. In the other bindings we don't set up a log handler by default, it just works the same way as the C API.> 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?Yup, it was already clear to me why these wouldn't work after I thought about it a bit more. My issue for maintenance is that if we add a new "poll-like" function to the API then the Rust bindings would need a special change.> 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.Sure, thanks! 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