Re: [Qemu-devel] [PATCH 2/2] QMP: Require 'use_unstable' arg for capabilities negotiation

[Luiz Capitulino]

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.
>