Eric Blake <ebl...@redhat.com> wrote:
> 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 <quint...@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.
We are using MigrationStatus from qapi-schema.json, add the comment
stating that.
>
>> + "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.
See above.
>
>
>> +++ 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.
It is wrong documentation, sorry for the inconvenience.
Later, Juan.
