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.

And on the eleven version I did, I put the strings wrong.  Results are
the same than from query-migrate.


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

I forget to do that when I changed it.

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

Bogus documentation, sorry.
This event it is only generated when we change the migration status to:
- SETUP, ACTIVE, COMPLETED, CANCELLING, CANCELLED and FAILED.

But let just say that this was my 1st big try with QMP events that have
arguments (previous version had one different event for each case, but
Jiri saw it and told me that it preffered it this way).

Thanks.