Luiz Capitulino <lcapitul...@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 <lcapitul...@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.
