Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

On 05/20/2015 09:35 AM, Juan Quintela wrote:
> We have one argument that tells us what event has happened.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>  migration/migration.c   | 12 ++++++++++++
>  qapi/event.json         | 14 ++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index 4c13d48..3797709 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -473,6 +473,22 @@ Example:
>  { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
>    "event": "SPICE_MIGRATE_COMPLETED" }
> 
> +MIGRATION
> +---------
> +
> +Emitted when a migration event happens
> +
> +Data: None.
> +
> + - "status": migration status
> +     "": error has been ignored

Uggh. Looking for an empty string is awkward.

> +     "report": error has been reported to the device
> +     "stop": the VM is going to stop because of the error
> +
> +Example:
> +
> +{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
> + "event": "MIGRATION", "data": {"status": "completed"}}

The example lists "completed", but the documentation does not mention
it. Might be good to expand the docs to mention all states, and/or point
to the enum definition.


> +++ b/qapi/event.json
> @@ -243,6 +243,20 @@
>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
> 
>  ##
> +# @MIGRATION
> +#
> +# Emitted when a migration event happens
> +#
> +# @status: @MigrationStatus describing the current migration status.
> +#          If this field is not returned, no migration process
> +#          has been initiated

Rather than returning an empty string,...

> +#
> +# Since: 2.4
> +##
> +{ 'event': 'MIGRATION',
> +  'data': {'status': 'MigrationStatus'}}

...this field should be marked optional, as in '*status'.  Then in your
callers, you'll have to pass true or false for has_status, so that you
can omit status when there is none.  But really, when will this event
ever be omitted if migration has not been initiated?  Maybe it is just
bogus documentation that you can return an empty string, as I didn't see
any addition of a call to qapi_event_send_migration() that would pass an
empty string on the wire.  So it sounds to me like the interface is
okay, but the documentation is wrong.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]