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

[Markus Armbruster]

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