[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] QMP: Require 'use_unstable' arg for capabil
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] QMP: Require 'use_unstable' arg for capabilities negotiation |
Date: |
Fri, 9 Jul 2010 09:50:17 -0300 |
On Fri, 09 Jul 2010 10:44:32 +0200
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> 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 <address@hidden>
> > ---
> > 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.
>