Re: [Qemu-devel] [PATCH v4 00/20] monitor: add asynchronous command type

[Markus Armbruster]

Marc-André, before you invest your time to answer my questions below: my
bandwidth for non-trivial QAPI features like this one is painfully
limited.  To get your QAPI conditionals in, I had to postpone other QAPI
projects.  I don't regret doing that, I'm rather pleased with how QAPI
conditionals turned out.  But I can't keep postponing all other QAPI
projects.  Because of that, this one will be slow going, at best.  Sorry
about that.

Marc-André Lureau <address@hidden> writes:

> Hi,
>
> HMP and QMP commands are handled synchronously in qemu today. But
> there are benefits allowing the command handler to re-enter the main
> loop if the command cannot be handled synchronously, or if it is
> long-lasting. Some bugs such as rhbz#1230527 are difficult to solve
> without it.
>
> The common solution is to use a pair of command+event in this case.

In particular, background jobs (qapi/jobs.json).  They grew out of block
jobs, and are still used only for "blocky" things.  Using them more
widely would probably make sense.

> But this approach has a number of issues:
> - you can't "fix" an existing command: you need a new API, and ad-hoc
>   documentation for that command+signal association, and old/broken
>   command deprecation

Making a synchronous command asynchronous is an incompatible change.  We
need to let the client needs opt in.  How is that done in this series?

> - since the reply event is broadcasted and 'id' is used for matching the
>   request, it may conflict with other clients request 'id' space

Any event that does that now is broken and needs to be fixed.  The
obvious fix is to include a monitor ID with the command ID.  For events
that can only ever be useful in the context of one particular monitor,
we could unicast to that monitor instead; see below.

Corollary: this is just a fixable bug, not a fundamental advantage of
the async feature.

> - it is arguably less efficient and elegant (weird API, useless return
>   in most cases, broadcast reply, no cancelling on disconnect etc)

The return value is useful for synchronously reporting failure to start
the background task.  I grant you that background tasks may exist that
won't ever fail to start.  I challenge the idea that it's most of them.

Broadcast reply could be avoided by unicasting events.  If I remember
correctly, Peter Xu even posted patches some time ago.  We ended up not
using them, because we found a better solution for the problem at hand.
My point is: this isn't a fundamental problem, it's just the way we
coded things up.

What do you mean by "no cancelling on disconnect"?

I'm ignoring "etc" unless you expand it into something specific.

I'm also not taking the "weird" bait :)

> The following series implements an async command solution instead. By
> introducing a session context and a command return handler, it can:
> - defer the return, allowing the mainloop to reenter
> - return only to the caller (instead of broadcast events for reply)
> - optionnally allow cancellation when the client is gone
> - track on-going qapi command(s) per client/session
>
> and without introduction of new QMP APIs or client visible change.

What do async commands provide that jobs lack?

Why do we want both?

I started to write a feature-by-feature comparison, but realized I don't
have the time to figure out either jobs or async from their (rather
sparse) documentation, let alone from code.

> Existing qemu commands can be gradually replaced by async:true
> variants when needed, while carefully reviewing the concurrency
> aspects. The async:true commands marshaller helpers are splitted in
> half, the calling and return functions. The command is called with a
> QmpReturn context, that can return immediately or later, using the
> generated return helper, which allows for a step-by-step conversion.
>
> The screendump command is converted to an async:true version to solve
> rhbz#1230527. The command shows basic cancellation (this could be
> extended if needed). It could be further improved to do asynchronous
> IO writes as well.

What is "basic cancellation"?

What extension(s) do you have in mind?

What's the impact of screendump writing synchronously?