5:20 a.m.
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.