Re: [Qemu-devel] [PATCH COLO-Frame v8 31/34] COLO: Add colo-set-checkpoint-period command

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2015/8/29 6:26, Eric Blake wrote:
> On 07/29/2015 02:45 AM, zhanghailiang wrote:
>> With this command, we can control the period of checkpoint, if
>> there is no comparison of net packets.
>>
>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>   hmp-commands.hx        | 15 +++++++++++++++
>>   hmp.c                  |  7 +++++++
>>   hmp.h                  |  1 +
>>   migration/colo.c       | 11 ++++++++++-
>>   qapi-schema.json       | 13 +++++++++++++
>>   qmp-commands.hx        | 22 ++++++++++++++++++++++
>>   stubs/migration-colo.c |  4 ++++
>>   7 files changed, 72 insertions(+), 1 deletion(-)
>
> Interface review:
>
>> +++ b/qapi-schema.json
>> @@ -691,6 +691,19 @@
>>   { 'command': 'colo-lost-heartbeat' }
>>
>>   ##
>> +# @colo-set-checkpoint-period
>> +#
>> +# Set colo checkpoint period
>> +#
>> +# @value: period of colo checkpoint in ms
>> +#
>> +# Returns: nothing on success
>
> Redundant line; you could omit it.
>
>> +#
>> +# Since: 2.4
>
> 2.5
>
>> +##
>> +{ 'command': 'colo-set-checkpoint-period', 'data': {'value': 'int'} }
>
> I hate write-only interfaces; where can I query the current period?
>

Yes, it is not graceful, actually, this should be convert to use migrate-set-parameters/query-migrate-parameters
commands to set/get the value, just as Dave's "[RFC/COLO:  1/3] COLO: Hybrid mode" patch does. But
these two commands should be reconstruct. Markus has promised to do this.

Hi Markus, is it still in your schedule ?

I will convert command to use  migrate-set-parameters/query-migrate-parameters temporarily for next version. thanks.

>> +++ b/stubs/migration-colo.c
>> @@ -52,3 +52,7 @@ void qmp_colo_lost_heartbeat(Error **errp)
>>                        " with --enable-colo option in order to support"
>>                        " COLO feature");
>>   }
>> +
>> +void qmp_colo_set_checkpoint_period(int64_t value, Error **errp)
>> +{
>> +}
>
> Shouldn't the stub function set an error, rather than being a no-op?
>

Yes, we should set an error, will fix in next version, thanks.