Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> Based-on: <address@hidden>

Now in master.

> This work is based on Markus's latest out-of-band fixes:
>   "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"
>
> Major stuff: some cleanups that were previously suggested by Markus or
> Eric.  Meanwhile fix up the flow control issue.  Since my proposal
> here is to drop COMMAND_DROPPED event, then we don't need to introduce
> per-monitor event emission API any more.  Though Markus told me that
> he might use that code somewhere else, so I'll post that per-monitor
> event code out separately as RFC later.
>
> Patch 1-3: cleanups.

I expect these to be ready in the next version.

Since it's just cleanup, there's no need to rush these into 3.0 unless
they enable something we want in 3.0.

> Patch 4-7: solve the flow control issue reported by Markus that
> response queue might overflow even if we have limitation on the
> request queue.  Firstly we drop the COMMAND_DROP event since that
> won't work for response queue (since queuing an event will need to
> append to the response queue itself), then we use monitor suspend and
> resume to control the flow.  Last, we apply that to response queue
> too.

These need work.  We need to agree on how exactly flow control should
work.  Moreover, we need to reconcile your work with Marc-André's
"[PATCH 00/12] RFC: monitor: various code simplification and fixes"
(which I haven't fully reviewed yet).

> Patch 8-9: the original patches to enable out-of-band by default.

I figure these patches are good; we just need to decide whether we're
ready to enable OOB.  Let's review the known issues.

* We might want to make "id" mandatory with exec-oob

  Best to do that right in the first release that fully supports OOB.

* OOB offered but rejected by client is not obviously the same as
  OOB not offered

  I still need to digest and reply to your
  Message-ID: <address@hidden>

* COMMAND_DROPPED is broken by design, and
* Flow control limits only request queue; response buffer can still
  grow without bounds

  You proposed to drop COMMAND_DROPPED, and do flow control by corking
  input, see above.

  Getting rid of broken COMMAND_DROPPED is a blocker.  Fully functional
  flow control isn't, since the QMP client is trusted.

* We lack test coverage for flow control
* test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c

  I'm inclined to treating lack of test coverage as a blocker.

* scripts/qmp/ doesn't support OOB, yet.  qmp-shell.py in particular.

  Not a blocker.

Whatever we don't address right away we should at least mark FIXME in
the source code.

Assuming my list is complete, and my assessments correct, then we're
quite close to the point where we can enable OOB.  But since rc1 is
tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
understand we need it sooner rather than later for postcopy recovery.
However, the current state should be servicable for teaching OOB to
libvirt: just follow the recommendation to supply "id" (libvirt always
does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
control isn't an issue.

[...]