Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Paolo Bonzini]

On Wed, Feb 12, 2025 at 1:47 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > > +pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
> > > +    let waker = Arc::new(RunFutureWaker {
> > > +        co: unsafe { bindings::qemu_coroutine_self() },
> > > +    })
> > > +    .into();
> >
> > into what? :)  Maybe you can add the type to the "let" for clarity.
>
> Into Waker. Do we have any idea yet how explicit we want to be with type
> annotations that aren't strictly necessary? I didn't think of making it
> explicit here because the only thing that is done with it is building a
> Context from it, so it seemed obvious enough.
>
> If you think that being explicit is better, then Waker::from() might
> be better than adding a type name to let and keeping .into().

I think this case threw me off because From<Arc<W: Wake>> is a bit
more generic than your
usual From implementation.  Maybe it's obvious to anyone who has used
futures in Rust
(I haven't).

I agree, something like

    let waker = Waker::from(waker);
    let mut cx = Context::from_waker(&waker);

could be the best of both worlds.

> > Also, would qemu_co_run_future() and qemu_run_future() become methods on an
> > Executor later?  Maybe it make sense to have already something like
> >
> > pub trait QemuExecutor {
> >     fn run_until<F: Future>(future: F) -> F::Output;
> > }
> >
> > pub struct Executor;
> > pub struct CoExecutor;
> >
> > and pass an executor to Rust functions (&Executor for no_coroutine_fn,
> > &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed).  Or would that
> > be premature in your opinion?
>
> If we could get bindgen to actually do that for the C interface, then
> this could be interesting, though for most functions it also would
> remain unused boilerplate. If we have to get the executor manually on
> the Rust side for each function, that's probably the same function that
> will want to execute the future - in which case it just can directly
> call the correct function.

The idea was that you don't call the correct function but the *only*
function :) i.e. exec.run_until(), and it will do the right thing for
coroutine vs. no coroutine.

But yeah, there would be boilerplate to pass it all the way down so
maybe it is not a great idea. I liked the concept that you just
*couldn't* get _co_ wrong... but perhaps it is not necessary once more
of "BlockDriver::open"
is lifted into bdrv_open<D: BlockDriver>.

Paolo

> The long term idea should be anyway that C coroutines disappear from the
> path and we integrate an executor into the QEMU main loop. But as long
> as the block layer core is written in C and uses coroutines and we want
> Rust futures to be able to call into coroutine_fns, that's still a long
> way to go.