Eric Blake <ebl...@redhat.com> writes:
> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
Thanks!
