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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
