| Message ID | 1278454757-5493-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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. >
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. [...]
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.
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.
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
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(-)