Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Eric Blake]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hmp-commands.hx  |  3 +--
>  hmp.c            | 17 +++++++++++++++++
>  hmp.h            |  1 +
>  monitor.c        | 42 ++++++++++++++++++------------------------
>  qapi-schema.json | 20 ++++++++++++++++++++
>  qmp-commands.hx  |  2 +-
>  6 files changed, 58 insertions(+), 27 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -638,6 +638,26 @@
>    'returns': 'MigrationParameters' }
>  
>  ##
> +# @client_migrate_info

Should we name this new command 'client-migrate-info'?

/me goes and checks...

Oh, wow, the QMP command IS advertised by 'query-commands' in spite of
not having this wrapper, even in qemu 2.3.

> +#
> +# Set the spice/vnc connection info for the migration target.  The
> +# spice/vnc server will ask the spice/vnc client to automatically
> +# reconnect using the new parameters (if specified) once the vm
> +# migration finished successfully.  Not yet implemented for VNC.
> +#
> +# @protocol:     must be "spice"
> +# @hostname:     migration target hostname
> +# @port:         #optional spice/vnc tcp port for plaintext channels

Is it worth documenting vnc, when we just stated earlier that protocol
must be spice?

> +# @tls-port:     #optional spice tcp port for tls-secured channels
> +# @cert-subject: #optional server certificate subject
> +#
> +# Since: 0.14.0

So this 'Since:' designation is correct, and we are just _finally_
documenting something that has silently been sitting around in QMP for a
looooong time.

> +##
> +{ 'command': 'client_migrate_info',
> +  'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int',
> +            '*tls-port': 'int', '*cert-subject': 'str' } }

Idea for followups - since 'protocol' must be "spice", should we:
1) make it an enum type rather than open-coded str
2) make it an optional parameter, so that omitting it defaults to spice?

> +
> +##
>  # @MouseInfo:
>  #
>  # Information about a mouse device.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c267c89..4611b6b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -785,7 +785,7 @@ EQMP
>          .args_type  = 
> "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>          .params     = "protocol hostname port tls-port cert-subject",
>          .help       = "send migration info to spice/vnc client",
> -        .mhandler.cmd_new = client_migrate_info,
> +        .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,

How many entries in qmp-commands.hx are NOT present in qapi? It looks
like this patch gets us one step closer to ditching the need to maintain
this file, which is a good thing.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature