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
Tage Johansson
2023-Jul-21 10:20 UTC
[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. >