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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Yup.  query-commands uses qmp_cmds[], not QAPI.

>> +#
>> +# 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?

I think this is a question for Gerd (cc'ed).

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

It's always been documented in qmp-commands.hx.

qmp-commands.hx predates QAPI.  We still use it to generate
qmp-commands.txt and qmp-commands-old.h.  The latter provides the
initializer for qmp_cmds[], thus determines the reply to query-commands.

qmp-commands.hx is complete.  The QAPI schema contains only qapified
commands, and therefore cannot serve as single authoritative source.

An awful lot of information is duplicated between qmp-commands.hx and
the QAPI schema.  To clean up this mess, we need to finish qapifying
commands, and then fold qmp-commands.hx's functionality into the schema.

>> +##
>> +{ '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

Useful cleanup.

> 2) make it an optional parameter, so that omitting it defaults to spice?

Not sure.  Gerd?

>> +
>> +##
>>  # @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?

Not many, but I don't remember the exact list offhand.  The most
difficult one is probably device_add.  I got patches getting that one a
bit closer, but they depend by a straightforward PPC patch I've been
trying to get in since February.

>                                                              It looks
> like this patch gets us one step closer to ditching the need to maintain
> this file, which is a good thing.

Yes.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!