Re: [Qemu-devel] monitor: enable OOB by default

[Peter Xu <peterx@redhat.com>]

On Thu, Jun 28, 2018 at 08:55:48AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jun 27, 2018 at 01:23:07PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Wed, Jun 27, 2018 at 10:41:38AM +0200, Markus Armbruster wrote:
> >> >> Markus Armbruster <armbru@redhat.com> writes:
> >> >> 
> >> >> > Markus Armbruster <armbru@redhat.com> writes:
> >> >> >
> >> >> >> I fooled around a bit, and I think there are a few lose ends.
> >> >> > [...]
> >> >> >> Talking to a QMP monitor that supports OOB:
> >> >> >>
> >> >> >>     $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
> >> >> >>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
> >> >> >>     QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
> >> >> >>     {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
> >> >> >>     QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
> >> >> >>     {"return": {}}
> >> >> >>     QMP> { "execute": "query-qmp-schema" }
> >> >> >>     {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}
> >> >> >>
> >> >> >> Why does every command require 'id'?
> >> >> >
> >> >> > I found one reason: event COMMAND_DROPPED wants it.  Any other reason?
> >> >> >
> >> >> > [...]
> >> >> 
> >> >> Apropos COMMAND_DROPPED: we send an event rather than an error response
> >> >> because we may send it out-of-order.  Makes sense.
> >> >> 
> >> >> However, broadcasting it to all monitors doesn't make sense.  We could
> >> >> use a way to send an event to just one monitor.
> >
> > True.
> >
> > (Sorry for the late responses; I was on Linuxcon China in the past few
> > days)
> 
> No need to be sorry :)
> 
> >> >
> >> > Worse than that - broadcasting to all monitors is categorically broken.
> >> > Different monitors make use the same "id" formatting scheme, so if you
> >> > broadcast COMMAND_DROPPED to a different monitor you might have clashing
> >> > "id" and thus incorrectly tell a client its command was dropped when in
> >> > fact it was processed. You'd have to be fairly unlucky in timing, but
> >> > it could happen.
> >> 
> >> Right.  Must fix bug.
> >
> > Even more true.
> >
> >> 
> >> I'm glad I went over this one more time, and in public!
> >
> > I had a glance at current qmp-spec, it seems that we don't have any
> > restriction currently on "we must send events to all the monitors".
> > Does it mean that we should be free to have per-monitor events
> > starting from this event?
> 
> Changing an existing event from broadcast to unicast is an observable
> change in existing behavior.  Compatibility break unless we can show
> nobody can use / uses the observation.
> 
> Creating a new event is not a compatibility break by itself[*],
> regardless of broadcast vs. unicast.

Yeah.

Though this change will be a bit unique in that basically we should
not break anyone but fix potential problems that the clients might
encounter.

Firstly I really suspect anyone has really started to use OOB
commands...  But let's just assume there is.

Then if a client has implemented the COMMAND_DROPPED event, our change
should still keep that event be there when any of the command is
dropped for that monitor.  What we really changed in the behavior is
that we won't send incorrect COMMAND_DROPPED events to them which
should not belong to them.  From that point of view, it's not really
the spec/api that is changed, we just try to fix a bug from a broken
implementation (from me) of the spec. :)

So if you would agree I'd prefer to directly change that bit to change
COMMAND_DROPPED as per-monitor event.  It should only benefit existing
clients (from not receiving broken events) rather than breaking any of
them.

> 
> > My current plan is that I can touch up scripts/qapi/events.py and
> > related stuff to allow QMPEventFuncEmit to take a monitor parameter,
> > then we pass in NULL when we want to send the event to all monitors.
> >
> > Would that work?
> 
> Think so.
> 
> Alternatively, a pair of functions:
> 
>     void qapi_event_bcast_EVENT(... event params ..., Error **errp);
>     void qapi_event_send_EVENT(Monitor *mon, ... event params ..., Error **errp);
> 
> Slightly more compact in the broadcast case, might be a bit easier to
> read.

Yeah I can try this.

> 
> 
> [*] In the case of COMMAND_DROPPED, the compatibility break is dropping
> commands (the event's trigger), caused by the command queuing feature.
> That's why command queuing has to be opt-in.  Details discussed
> elsewhere in this thread.

Thanks,

-- 
Peter Xu