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

[Eric Blake]

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 <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  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/

> +             * 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).

> @@ -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.


> +++ 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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature