| Message ID | 20180702162218.13678-8-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qmp: Fixes and cleanups around OOB commands | expand |
On 07/02/2018 11:21 AM, Markus Armbruster wrote: > Commit cf869d53172 "qmp: support out-of-band (oob) execution" made > "id" mandatory for all commands when the client accepted capability > "oob". This is rather onerous when you play with QMP by hand, and > unnecessarily so: only out-of-band commands need an ID for reliable > matching of response to command. > > Revert that part of commit cf869d53172 for now. We may still make > "id" mandatory for out-of-band commands. Fair enough for now (I still think mandatory "id" for oob commands is worthwhile, but I also think "exec-oob" is worthwhile, and it's probably easier to write the logic for mandating "id" after that tweak). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > docs/interop/qmp-spec.txt | 9 +++------ > monitor.c | 7 ------- > 2 files changed, 3 insertions(+), 13 deletions(-) > > +++ b/docs/interop/qmp-spec.txt > @@ -103,16 +103,13 @@ The format for command execution is: > required. Each command documents what contents will be considered > valid when handling the json-argument > - The "id" member is a transaction identification associated with the > - command execution. It is required for all commands if the OOB - > - capability was enabled at startup, and optional otherwise. The same > - "id" field will be part of the response if provided. The "id" > - member can be any json-value. A json-number incremented for each > - successive command works fine. > + command execution, it is optional and will be part of the response > + if provided. The "id" member can be any json-value. A json-number > + incremented for each successive command works fine. > - The optional "control" member further specifies how the command is > to be executed. Currently, its only member is optional "run-oob". > See section "2.3.1 Out-of-band execution" for details. We should STILL recommend that a client should use "id" when using oob, whether or not we mandate it later. > > - > 2.3.1 Out-of-band execution > --------------------------- > > diff --git a/monitor.c b/monitor.c > index 96e87d8664..b7d74b01b4 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4291,13 +4291,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) > > id = qdict_get(qdict, "id"); > > - /* When OOB is enabled, the "id" field is mandatory. */ > - if (qmp_oob_enabled(mon) && !id) { > - error_setg(&err, "Out-of-band capability requires that " > - "every command contains an 'id' field"); > - goto err; > - } I can live with this hunk right away, but the documentation feels weak enough that I'm reluctant to give R-b to the patch as-is
On Mon, Jul 02, 2018 at 06:21:53PM +0200, Markus Armbruster wrote: > Commit cf869d53172 "qmp: support out-of-band (oob) execution" made > "id" mandatory for all commands when the client accepted capability > "oob". This is rather onerous when you play with QMP by hand, and > unnecessarily so: only out-of-band commands need an ID for reliable > matching of response to command. > > Revert that part of commit cf869d53172 for now. We may still make > "id" mandatory for out-of-band commands. This change should be okay with current implementation when out-of-band commands are still in order themselves, though I'm still not that confident on whether we really want this change if only for the sake of easier usage for human beings. If we see Libvirt, the real player for QMP - it has the "id" field even for in-band commands always. I'd say the "id" field is really helpful for machines, though not that friendly to us. Basically I'll read it as: machines like "id"s, humans hate "id"s. And QMP is Qemu Machine Protocol after all... so not sure whether it'll be good we change that for us humans. Regards,
Eric Blake <eblake@redhat.com> writes: > On 07/02/2018 11:21 AM, Markus Armbruster wrote: >> Commit cf869d53172 "qmp: support out-of-band (oob) execution" made >> "id" mandatory for all commands when the client accepted capability >> "oob". This is rather onerous when you play with QMP by hand, and >> unnecessarily so: only out-of-band commands need an ID for reliable >> matching of response to command. >> >> Revert that part of commit cf869d53172 for now. We may still make >> "id" mandatory for out-of-band commands. > > Fair enough for now (I still think mandatory "id" for oob commands is > worthwhile, but I also think "exec-oob" is worthwhile, and it's > probably easier to write the logic for mandating "id" after that > tweak). > >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> docs/interop/qmp-spec.txt | 9 +++------ >> monitor.c | 7 ------- >> 2 files changed, 3 insertions(+), 13 deletions(-) >> > >> +++ b/docs/interop/qmp-spec.txt >> @@ -103,16 +103,13 @@ The format for command execution is: >> required. Each command documents what contents will be considered >> valid when handling the json-argument >> - The "id" member is a transaction identification associated with the >> - command execution. It is required for all commands if the OOB - >> - capability was enabled at startup, and optional otherwise. The same >> - "id" field will be part of the response if provided. The "id" >> - member can be any json-value. A json-number incremented for each >> - successive command works fine. >> + command execution, it is optional and will be part of the response >> + if provided. The "id" member can be any json-value. A json-number >> + incremented for each successive command works fine. >> - The optional "control" member further specifies how the command is >> to be executed. Currently, its only member is optional "run-oob". >> See section "2.3.1 Out-of-band execution" for details. > > We should STILL recommend that a client should use "id" when using > oob, whether or not we mandate it later. Yes. I'll take care of it in v2. > >> - >> 2.3.1 Out-of-band execution >> --------------------------- >> diff --git a/monitor.c b/monitor.c >> index 96e87d8664..b7d74b01b4 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -4291,13 +4291,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) >> id = qdict_get(qdict, "id"); >> - /* When OOB is enabled, the "id" field is mandatory. */ >> - if (qmp_oob_enabled(mon) && !id) { >> - error_setg(&err, "Out-of-band capability requires that " >> - "every command contains an 'id' field"); >> - goto err; >> - } > > I can live with this hunk right away, but the documentation feels weak > enough that I'm reluctant to give R-b to the patch as-is
Peter Xu <peterx@redhat.com> writes: > On Mon, Jul 02, 2018 at 06:21:53PM +0200, Markus Armbruster wrote: >> Commit cf869d53172 "qmp: support out-of-band (oob) execution" made >> "id" mandatory for all commands when the client accepted capability >> "oob". This is rather onerous when you play with QMP by hand, and >> unnecessarily so: only out-of-band commands need an ID for reliable >> matching of response to command. >> >> Revert that part of commit cf869d53172 for now. We may still make >> "id" mandatory for out-of-band commands. > > This change should be okay with current implementation when > out-of-band commands are still in order themselves, though I'm still > not that confident on whether we really want this change if only for > the sake of easier usage for human beings. > > If we see Libvirt, the real player for QMP - it has the "id" field > even for in-band commands always. I'd say the "id" field is really > helpful for machines, though not that friendly to us. > > Basically I'll read it as: machines like "id"s, humans hate "id"s. > And QMP is Qemu Machine Protocol after all... so not sure whether > it'll be good we change that for us humans. "id" being optional doesn't hurt libvirt in any way. Thus, I see no need to inconvenience humans. Daniel has argued[*] for making "id" mandatory with OOB commands. I'm not rejecting that argument. But I needed to get this out in a hurry, and simply reverting something is quicker than debating and implementing an improvement. There's still time to tweak this before the release. [*] Message-ID: <20180628120044.GF3513@redhat.com> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08322.html
On Tue, Jul 03, 2018 at 08:14:35AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Mon, Jul 02, 2018 at 06:21:53PM +0200, Markus Armbruster wrote: > >> Commit cf869d53172 "qmp: support out-of-band (oob) execution" made > >> "id" mandatory for all commands when the client accepted capability > >> "oob". This is rather onerous when you play with QMP by hand, and > >> unnecessarily so: only out-of-band commands need an ID for reliable > >> matching of response to command. > >> > >> Revert that part of commit cf869d53172 for now. We may still make > >> "id" mandatory for out-of-band commands. > > > > This change should be okay with current implementation when > > out-of-band commands are still in order themselves, though I'm still > > not that confident on whether we really want this change if only for > > the sake of easier usage for human beings. > > > > If we see Libvirt, the real player for QMP - it has the "id" field > > even for in-band commands always. I'd say the "id" field is really > > helpful for machines, though not that friendly to us. > > > > Basically I'll read it as: machines like "id"s, humans hate "id"s. > > And QMP is Qemu Machine Protocol after all... so not sure whether > > it'll be good we change that for us humans. > > "id" being optional doesn't hurt libvirt in any way. Thus, I see no > need to inconvenience humans. > > Daniel has argued[*] for making "id" mandatory with OOB commands. I'm > not rejecting that argument. But I needed to get this out in a hurry, > and simply reverting something is quicker than debating and implementing > an improvement. There's still time to tweak this before the release. > > > [*] Message-ID: <20180628120044.GF3513@redhat.com> > https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08322.html Okay, it's fine to me then, especially if Daniel won't have other opinions. Regards,
On Tue, Jul 03, 2018 at 08:14:35AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Mon, Jul 02, 2018 at 06:21:53PM +0200, Markus Armbruster wrote: > >> Commit cf869d53172 "qmp: support out-of-band (oob) execution" made > >> "id" mandatory for all commands when the client accepted capability > >> "oob". This is rather onerous when you play with QMP by hand, and > >> unnecessarily so: only out-of-band commands need an ID for reliable > >> matching of response to command. > >> > >> Revert that part of commit cf869d53172 for now. We may still make > >> "id" mandatory for out-of-band commands. > > > > This change should be okay with current implementation when > > out-of-band commands are still in order themselves, though I'm still > > not that confident on whether we really want this change if only for > > the sake of easier usage for human beings. > > > > If we see Libvirt, the real player for QMP - it has the "id" field > > even for in-band commands always. I'd say the "id" field is really > > helpful for machines, though not that friendly to us. > > > > Basically I'll read it as: machines like "id"s, humans hate "id"s. > > And QMP is Qemu Machine Protocol after all... so not sure whether > > it'll be good we change that for us humans. > > "id" being optional doesn't hurt libvirt in any way. Thus, I see no > need to inconvenience humans. > > Daniel has argued[*] for making "id" mandatory with OOB commands. I'm > not rejecting that argument. But I needed to get this out in a hurry, > and simply reverting something is quicker than debating and implementing > an improvement. There's still time to tweak this before the release. I'm fine with this - I agree that it is more important to avoid creating a regression, than to enforce "id" for OOB. We've still documented that OOB will require "id", so it would not be a surprise once we enforce it later. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt index e5f8116c54..794c95838f 100644 --- a/docs/interop/qmp-spec.txt +++ b/docs/interop/qmp-spec.txt @@ -103,16 +103,13 @@ The format for command execution is: required. Each command documents what contents will be considered valid when handling the json-argument - The "id" member is a transaction identification associated with the - command execution. It is required for all commands if the OOB - - capability was enabled at startup, and optional otherwise. The same - "id" field will be part of the response if provided. The "id" - member can be any json-value. A json-number incremented for each - successive command works fine. + command execution, it is optional and will be part of the response + if provided. The "id" member can be any json-value. A json-number + incremented for each successive command works fine. - The optional "control" member further specifies how the command is to be executed. Currently, its only member is optional "run-oob". See section "2.3.1 Out-of-band execution" for details. - 2.3.1 Out-of-band execution --------------------------- diff --git a/monitor.c b/monitor.c index 96e87d8664..b7d74b01b4 100644 --- a/monitor.c +++ b/monitor.c @@ -4291,13 +4291,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) id = qdict_get(qdict, "id"); - /* When OOB is enabled, the "id" field is mandatory. */ - if (qmp_oob_enabled(mon) && !id) { - error_setg(&err, "Out-of-band capability requires that " - "every command contains an 'id' field"); - goto err; - } - req_obj = g_new0(QMPRequest, 1); req_obj->mon = mon; req_obj->id = qobject_ref(id);
Commit cf869d53172 "qmp: support out-of-band (oob) execution" made "id" mandatory for all commands when the client accepted capability "oob". This is rather onerous when you play with QMP by hand, and unnecessarily so: only out-of-band commands need an ID for reliable matching of response to command. Revert that part of commit cf869d53172 for now. We may still make "id" mandatory for out-of-band commands. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/interop/qmp-spec.txt | 9 +++------ monitor.c | 7 ------- 2 files changed, 3 insertions(+), 13 deletions(-)