On Fri, Jan 12, 2018 at 02:57:23PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > After this patch, we will allow QMP clients to enable QMP capabilities
> > when sending the first "qmp_capabilities" command. Originally we are
> > starting QMP session with no arguments like:
> >
> > { "execute": "qmp_capabilities" }
> >
> > Now we can enable some QMP capabilities using (take OOB as example,
> > which is the only one capability that we support):
> >
> > { "execute": "qmp_capabilities",
> > "argument": { "enable": [ "oob" ] } }
> >
> > When the "argument" key is not provided, no capability is enabled.
> >
> > For capability "oob", the monitor needs to be run on dedicated IO
> > thread, otherwise the command will fail. For example, trying to enable
> > OOB on a MUXed typed QMP monitor will fail.
>
>
> >
> > One thing to mention is that, QMP capabilities are per-monitor, and also
> > when the connection is closed due to some reason, the capabilities will
> > be reset.
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > monitor.c | 65
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > qapi-schema.json | 15 ++++++++++---
> > 2 files changed, 74 insertions(+), 6 deletions(-)
> >
>
> > @@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
> > return;
> > }
> >
> > + /* Enable QMP capabilities provided by the guest if applicable. */
> > + if (has_enable) {
> > + qmp_caps_check(cur_mon, enable, &local_err);
> > + if (local_err) {
> > + /*
> > + * Failed check on either of the capabilities will fail
>
> s/either/any/
Fixed.
>
> > + * the apply of all.
>
> s/apply/application/
>
> or even a more verbose
>
> will fail the entire command (and thus not apply any of the other
> capabilities that were also requested).
Sure.
>
> > @@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
> > qmp_marshal_query_version(NULL, &ver, NULL);
> >
> > for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > + if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
> > + /* Monitors that are not using IOThread won't support OOB */
> > + continue;
> > + }
> > qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
>
> I think this hunk belongs in the previous patch - the server should not
> advertise 'oob' in the greeting if it will not be able to honor it.
Yes, Fam asked the same question. How about I squash these two
patches? After all chunk moving between commits are even more
error-prone to me... and even moving the chunk will need me to drop
all r-bs for the patches.
Let me squash them.
>
>
> > +++ b/qapi-schema.json
> > @@ -102,21 +102,30 @@
> > #
> > # Enable QMP capabilities.
> > #
> > -# Arguments: None.
> > +# Arguments:
> > +#
> > +# @enable: List of QMPCapabilities to enable, which is optional.
>
> Maybe document that this list must not include any capabilities that
> were not mentioned in the server's initial greeting.
Ok. Thanks,
--
Peter Xu
