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

[Vlad Yasevich <vyasevic@redhat.com>]

On 05/30/2017 05:58 AM, Juan Quintela wrote:
> Vladislav Yasevich <vyasevic@redhat.com> 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>
> 
> Hi
> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f5e8194..cee2837 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -78,6 +78,104 @@ static struct mig_cmd_args {
>>      [MIG_CMD_MAX]              = { .len = -1, .name = "MAX" },
>>  };
>>  
> 
> Once that you are touching this, shouldn't it be better to be in
> net/<somewhere>?
> They have nothing to do with migration really.
> 

Yeah, I considered this, but could really find a good slot for them.
I can either put them into net.c directly or pull them out into their
own little file.

> 
>> +#define QEMU_ANNOUNCE_INITIAL    50
>> +#define QEMU_ANNOUNCE_MAX       550
>> +#define QEMU_ANNOUNCE_ROUNDS      5
>> +#define QEMU_ANNOUNCE_STEP      100
>> +
>> +AnnounceParameters *qemu_get_announce_params(void)
>> +{
>> +    static AnnounceParameters announce = {
>> +        .initial = QEMU_ANNOUNCE_INITIAL,
>> +        .max = QEMU_ANNOUNCE_MAX,
>> +        .rounds = QEMU_ANNOUNCE_ROUNDS,
>> +        .step = QEMU_ANNOUNCE_STEP,
>> +    };
>> +
>> +    return &announce;
>> +}
>> +
>> +void qemu_fill_announce_parameters(AnnounceParameters **to,
>> +                                   AnnounceParameters *from)
>> +{
>> +    AnnounceParameters *params;
>> +
>> +    params = *to = g_malloc0(sizeof(*params));
>> +    params->has_initial = true;
>> +    params->initial = from->initial;
>> +    params->has_max = true;
>> +    params->max = from->max;
>> +    params->has_rounds = true;
>> +    params->rounds = from->rounds;
>> +    params->has_step = true;
>> +    params->step = from->step;
>> +}
>> +
>> +bool qemu_validate_announce_parameters(AnnounceParameters *params, Error **errp)
>> +{
>> +    if (params->has_initial &&
>> +        (params->initial < 1 || params->initial > 100000)) {
> 
> This is independent of this patch, but we really need a macro:
>   CHECK(field, mininum, maximum)
> 
> We repeat this left and right.
> 
>> +void qemu_set_announce_parameters(AnnounceParameters *announce_params,
>> +                                  AnnounceParameters *params)
> 
>> +void qmp_announce_set_parameters(AnnounceParameters *params, Error **errp)
> 
> Really similar functions name.  I assume by know that the 1st function
> is used somewhere else in the series.
> 

Yes, the first function is going to be used later.

Thanks
-vlad