[Libguestfs] [libnbd PATCH 09/10] rust: async: Create an async friendly handle type
Richard W.M. Jones
rjones at redhat.com
Fri Jul 21 10:37:48 UTC 2023
On Fri, Jul 21, 2023 at 10:20:49AM +0000, Tage Johansson wrote:
>
> 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"?
Say we added "poll3" or something, we'd need to also remember to
change this list in the Rust bindings.
Rich.
>
> 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.
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
More information about the Libguestfs
mailing list