On Fri, 09 Jul 2010 10:44:32 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > This helps ensuring two things: > > > > 1. An initial warning on client writers playing with current QMP > > 2. Clients using unstable QMP will break when we declare QMP stable and > > drop that argument > > > > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com> > > --- > > QMP/README | 2 +- > > QMP/qmp-shell | 2 +- > > QMP/qmp.py | 3 +++ > > monitor.c | 7 ++++++- > > qemu-monitor.hx | 14 ++++++++++---- > > 5 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/QMP/README b/QMP/README > > index 30a283b..14d36ee 100644 > > --- a/QMP/README > > +++ b/QMP/README > > @@ -65,7 +65,7 @@ Trying 127.0.0.1... > > Connected to localhost. > > Escape character is '^]'. > > {"QMP": {"version": {"qemu": "0.12.50", "package": ""}, "capabilities": > > []}} > > -{ "execute": "qmp_capabilities" } > > +{ "execute": "qmp_capabilities", "arguments": { "use_unstable": true } } > > {"return": {}} > > { "execute": "query-version" } > > {"return": {"qemu": "0.12.50", "package": ""}} > > diff --git a/QMP/qmp-shell b/QMP/qmp-shell > > index a5b72d1..17033b1 100755 > > --- a/QMP/qmp-shell > > +++ b/QMP/qmp-shell > > @@ -42,7 +42,7 @@ def main(): > > > > qemu = qmp.QEMUMonitorProtocol(argv[1]) > > qemu.connect() > > - qemu.send("qmp_capabilities") > > + qemu.capabilities() > > > > print 'Connected!' > > > > diff --git a/QMP/qmp.py b/QMP/qmp.py > > index 4062f84..9d6f428 100644 > > --- a/QMP/qmp.py > > +++ b/QMP/qmp.py > > @@ -26,6 +26,9 @@ class QEMUMonitorProtocol: > > raise QMPConnectError > > return data['QMP']['capabilities'] > > > > + def capabilities(self): > > + self.send_raw('{ "execute": "qmp_capabilities", "arguments": { > > "use_unstable": true } }') > > + > > def close(self): > > self.sock.close() > > > > diff --git a/monitor.c b/monitor.c > > index 55633fd..19ddf1e 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -479,7 +479,12 @@ static int do_qmp_capabilities(Monitor *mon, const > > QDict *params, > > { > > /* Will setup QMP capabilities in the future */ > > if (monitor_ctrl_mode(mon)) { > > - mon->mc->command_mode = 1; > > + if (qdict_get_bool(params, "use_unstable")) { > > + mon->mc->command_mode = 1; > > + } else { > > + qerror_report(QERR_INVALID_PARAMETER, "use_unstable"); > > + return -1; > > + } > > } > > > > return 0; > > Unusual use of QERR_INVALID_PARAMETER. It's normally used to flag > invalid parameters *names*. The name is fine here, it's the value you > don't like. > > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > > index 2d2a09e..a56e1f5 100644 > > --- a/qemu-monitor.hx > > +++ b/qemu-monitor.hx > > @@ -1557,7 +1557,7 @@ EQMP > > > > { > > .name = "qmp_capabilities", > > - .args_type = "", > > + .args_type = "use_unstable:b", > > .params = "", > > .help = "enable QMP capabilities", > > .user_print = monitor_user_noop, > > @@ -1575,14 +1575,20 @@ qmp_capabilities > > > > Enable QMP capabilities. > > > > -Arguments: None. > > +Arguments: > > + > > +- use_unstable: really enable unstable version of QMP (json-bool) > > > > Example: > > > > --> { "execute": "qmp_capabilities" } > > +-> { "execute": "qmp_capabilities", "arguments": { "use_unstable": true } } > > <- { "return": {} } > > > > -Note: This command must be issued before issuing any other command. > > +Notes: > > + > > +(1) This command must be issued before issuing any other command. > > + > > +(2) Setting "use_unstable" to true is the only way to get anything working. > > > > EQMP > > Is it really necessary to break all existing users of QMP? The protocol is going to change, they will break anyway. > What are we trying to accomplish by that? QMP in 0.13 is in usable state. I fear that people will start using it without noting/caring the protocol is going to be different in 0.14. The removal of this flag in 0.14 (assuming we'll have a stable QMP by then), makes clients break right away, instead of unexpected breaking in subtle ways. This makes it easy to identify what's wrong and the message will be: you should review your QMP usage, because the protocol has changed. That said, I'm not that strong about this particular solution. What I really would like to have is an easy way to identify old clients using a now stable version of QMP. > Caution: there is an unstated anti-dependency on the new argument > checker. The new checker rejects unknown arguments, the old checker > doesn't. This leads to the following behaviors: > > Checker This patch use_unstable > old not applied doesn't matter > old applied must be there > new not applied must not be there (*) > new applied must be there > > If combination (*) exists, a client can't just pass use_unstable. > It needs to try both. Best to avoid that. >