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

On 7/21/2023 12:05 PM, Richard W.M. Jones wrote: >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? Yes, both for `libnbd::Handle` and `libnbd::AsyncHandle`. `libnbd::AsyncHandle` is built ontop of `libnbd::Handle`. >>```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. Sorry, but what do you mean by "would need a special change"?

Best regards, Tage >>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. >