Patchwork [2/2] QMP: Require 'use_unstable' arg for capabilities negotiation

login
register
mail settings
Submitter Luiz Capitulino
Date July 6, 2010, 10:19 p.m.
Message ID <1278454757-5493-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/58071/
State New
Headers show

Comments

Luiz Capitulino - July 6, 2010, 10:19 p.m.
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 <lcapitulino@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(-)
Markus Armbruster - July 9, 2010, 8:44 a.m.
Luiz Capitulino <lcapitulino@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 <lcapitulino@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?  What are we
trying to accomplish by that?

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.
Luiz Capitulino - July 9, 2010, 12:50 p.m.
On Fri, 09 Jul 2010 10:44:32 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@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 <lcapitulino@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.
>
Markus Armbruster - July 9, 2010, 1:24 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 09 Jul 2010 10:44:32 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@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
[...]
>> Is it really necessary to break all existing users of QMP?
>
> The protocol is going to change, they will break anyway.

Then why break them now in addition to then?

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

If we want obsolete clients to break when we release 0.14, then let's
break them then.  No need to break not-yet-obsolete clients now.
Especially not in a way that unbreaks them in 0.14, when they are
*really* obsolete.

[...]
Luiz Capitulino - July 9, 2010, 1:36 p.m.
On Fri, 09 Jul 2010 15:24:48 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 09 Jul 2010 10:44:32 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@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
> [...]
> >> Is it really necessary to break all existing users of QMP?
> >
> > The protocol is going to change, they will break anyway.
> 
> Then why break them now in addition to then?

Existing clients? Yes, you have a point. Although our only known existing
client atm is libvirt.

> >> 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.
> 
> If we want obsolete clients to break when we release 0.14, then let's
> break them then.  No need to break not-yet-obsolete clients now.
> Especially not in a way that unbreaks them in 0.14, when they are
> *really* obsolete.

I don't think we have that many clients today, the only known one is
libvirt. So, this patch takes only new clients in consideration, those
who might start using QMP in 0.13.

Again, I'm ok with a different solution. I just want to go beyond a README file
warning.

What about a "warning" key in the greeting message? Like:

 "warning": "QMP is unstable, it will break soon!"

Not sure it matters as our greeting message is a bit too verbose, but
seems better than nothing.
Markus Armbruster - July 9, 2010, 1:46 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 09 Jul 2010 15:24:48 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 09 Jul 2010 10:44:32 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@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
>> [...]
>> >> Is it really necessary to break all existing users of QMP?
>> >
>> > The protocol is going to change, they will break anyway.
>> 
>> Then why break them now in addition to then?
>
> Existing clients? Yes, you have a point. Although our only known existing
> client atm is libvirt.
>
>> >> 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.
>> 
>> If we want obsolete clients to break when we release 0.14, then let's
>> break them then.  No need to break not-yet-obsolete clients now.
>> Especially not in a way that unbreaks them in 0.14, when they are
>> *really* obsolete.
>
> I don't think we have that many clients today, the only known one is
> libvirt. So, this patch takes only new clients in consideration, those
> who might start using QMP in 0.13.
>
> Again, I'm ok with a different solution. I just want to go beyond a README file
> warning.
>
> What about a "warning" key in the greeting message? Like:
>
>  "warning": "QMP is unstable, it will break soon!"
>
> Not sure it matters as our greeting message is a bit too verbose, but
> seems better than nothing.

Works for me.

Patch

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