Re: [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 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.