Re: [Qemu-devel] [PATCH 01/12] migration: Introduce announce parameters

[Vlad Yasevich <vyasevic@redhat.com>]

On 05/26/2017 09:08 AM, Eric Blake wrote:
> On 05/24/2017 01:05 PM, Vladislav Yasevich wrote:
>> Add parameters that control RARP/GARP announcement timeouts.
>> The parameters structure is added to the QAPI and a qmp command
>> is added to set/get the parameter data.
>>
>> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
> 
> Just an interface review for now:
> 
>> +++ b/qapi-schema.json
>> @@ -569,6 +569,90 @@
>>  ##
>>  { 'command': 'query-events', 'returns': ['EventInfo'] }
>>  
>> +
>> +##
>> +# @AnnounceParameter:
>> +#
>> +# @AnnounceParameter enumeration
>> +#
>> +# @initial: Initial delay (in ms) before sending the first GARP/RARP
>> +#       announcement
>> +#
>> +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets
> 
> s/announcemnt/announcement/
> 
>> +#
>> +# @rounds: Number of self-announcement attempts
>> +#
>> +# @step: Delay increate (in ms) after each self-announcment attempt
> 
> s/increate/increase/
> s/announcment/announcement/
> 
>> +#
>> +# Since: 2.10
>> +##
>> +{ 'enum' : 'AnnounceParameter',
>> +  'data' : [ 'initial', 'max', 'rounds', 'step' ] }
> 
> Why are we creating an enum here?  Without reading further, it looks
> like you plan on using the enum to delineate members of a union? But
> that feels like it will be overly complicated.  A struct should be
> sufficient (each parameter being an optional member of the struct, where
> you can supply as many or as few on input, but all are reported on output).
> 

I end up using them for the HMP interface.  If it's better, I can move this
definition to the HMP patch.

Thanks
-vlad