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

[zhanghailiang]

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 <address@hidden>
> > Cc: Eric Blake <address@hidden>
> > Cc: Markus Armbruster <address@hidden>
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > ---
> >   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.