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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

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?

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]