Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Peter Xu <peterx@redhat.com>]

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 <dgilbert@redhat.com>
> > Signed-off-by: Peter Xu <peterx@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