[Qemu-devel] [PATCH 0/2]: QMP: ensure we will break unstable clients

[Qemu-devel] [PATCH 0/2]: QMP: ensure we will break unstable clients

[Luiz Capitulino]

First patch is a small README update, important change is in second patch.

Thanks.

[Qemu-devel] [PATCH 1/2] QMP: Update README file

[Luiz Capitulino]

A number of small changes I prefer to do in one shot:

- Add a note about instability
- Add multiple monitors example
- Small clarifications

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/README |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/QMP/README b/QMP/README
index 35a80c7..30a283b 100644
--- a/QMP/README
+++ b/QMP/README
@@ -7,13 +7,17 @@ Introduction
 The QEMU Monitor Protocol (QMP) allows applications to communicate with
 QEMU's Monitor.
 
-QMP is JSON[1] based and has the following features:
+QMP is JSON[1] based and currently has the following features:
 
 - Lightweight, text-based, easy to parse data format
-- Asynchronous events support 
-- Stability
+- Asynchronous messages support (ie. events)
+- Capabilities Negotiation
 
-For more information, please, refer to the following files:
+However, QMP is still under heavy development and is considered an unstable
+protocol. This means that its interface is going to have incompatible changes
+between QEMU releases. We plan to make QMP stable as soon as possible.
+
+For more information on QMP's usage, please, refer to the following files:
 
 o qmp-spec.txt      QEMU Monitor Protocol current specification
 o qmp-commands.txt  QMP supported commands
@@ -29,9 +33,8 @@ o vm-info         Show some information about the Virtual Machine
 Usage
 -----
 
-To enable QMP, QEMU has to be started in "control mode". There are
-two ways of doing this, the simplest one is using the the '-qmp'
-command-line option.
+To enable QMP, QEMU has to be started in "control mode". There are two ways of
+doing this, the simplest one is using the the '-qmp' command-line option.
 
 For example:
 
@@ -40,9 +43,17 @@ $ qemu [...] -qmp tcp:localhost:4444,server
 Will start QEMU in control mode, waiting for a client TCP connection
 on localhost port 4444.
 
-It is also possible to use the '-mon' command-line option to have
-more complex combinations. Please, refer to the QEMU's manpage for
-more information.
+To have more complex combinations, like multiple monitors, the '-mon'
+command-line option should be used with the '-chardev' one. For instance, the
+following command-line creates one user monitor on stdio and one QMP monitor
+on localhost port 4444.
+
+$ qemu [...] -chardev stdio,id=mon0 -mon chardev=mon0,mode=readline \
+             -mon chardev=mon0,mode=readline \
+             -chardev socket,id=mon1,host=localhost,port=4444,server \
+             -mon chardev=mon1,mode=control
+
+Please, refer to QEMU's manpage for more information.
 
 Simple Testing
 --------------
@@ -59,8 +70,7 @@ Escape character is '^]'.
 { "execute": "query-version" }
 {"return": {"qemu": "0.12.50", "package": ""}}
 
-Contact
--------
+Homepage
+--------
 
 http://www.linux-kvm.org/page/MonitorProtocol
-Luiz Fernando N. Capitulino <lcapitulino@redhat.com>
-- 
1.7.2.rc0

Re: [Qemu-devel] [PATCH 1/2] QMP: Update README file

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> A number of small changes I prefer to do in one shot:
>
> - Add a note about instability
> - Add multiple monitors example
> - Small clarifications

ACK for .13

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

[Luiz Capitulino]

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;
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
 
-- 
1.7.2.rc0

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

[Markus Armbruster]

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.

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

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

[Markus Armbruster]

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.

[...]

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

[Luiz Capitulino]

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.

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

[Markus Armbruster]

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.