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

[Juan Quintela]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  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.