Re: [Qemu-devel] [PATCH for-2.9 V2 2/3] Add a new qmp command to start/stop replication

[Zhang Chen]

On 12/08/2016 11:38 PM, Eric Blake wrote:
> On 12/08/2016 01:55 AM, Zhang Chen wrote:
> > We can call this qmp command to start/stop replication outside of qemu.
> > Like Xen colo need this function.
> >
> > Signed-off-by: Zhang Chen <address@hidden>
> > ---
> >   docs/qmp-commands.txt | 17 +++++++++++++++++
> >   migration/colo.c      | 22 ++++++++++++++++++++++
> >   qapi-schema.json      | 19 +++++++++++++++++++
> >   3 files changed, 58 insertions(+)
> >
> > diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> > index abf210a..f7bae0d 100644
> > --- a/docs/qmp-commands.txt
> > +++ b/docs/qmp-commands.txt
> > @@ -432,6 +432,23 @@ Example:
> >        "arguments": { "enable": true } }
> >   <- { "return": {} }
> >   
> > +xen-set-replication
> > +-------
> Much of this file tries to match the ----- divider lines to the length
> of the line above it (you're short by the length of '-replication')

OK~ I will add '----' match the length of '-replication',
and I find some command have same problem like 'xen-set-global-dirty-log',
Should I send another patch to fix it?

> > +
> > +Enable or disable replication.
> > +
> > +Arguments:
> > +
> > +- "enable": Enable it or disable it.
> > +- "primary": True for primary or false for secondary
> We aren't very consistent on whether to end in '.'

I will fix it.

> > +- "failover": Enable to do failover when stoping replcation
> "failover": Enable failover when stopping replication (optional, default
> false)

I got it.

> > +void qmp_xen_set_replication(bool enable, bool primary,
> > +                             bool has_failover, bool failover,
> > +                             Error **errp)
> > +{
> > +    ReplicationMode mode = primary ?
> > +                           REPLICATION_MODE_PRIMARY :
> > +                           REPLICATION_MODE_SECONDARY;
> > +
> > +    if (has_failover && enable) {
> > +        error_setg(errp, "Parameter 'failover' is only for"
> > +                   " stopping replication");
> At this point, errp is set, but you fall through...

I will add a 'return' here.

> > +    }
> > +
> > +    if (enable) {
> > +        replication_start_all(mode, errp);
> and could abort if something else also tries to set errp.  Simplest is
> to add a return; statement after error_setg().

Yes.

> > +    } else {
> > +        replication_stop_all(failover, failover ? NULL : errp);
> > +    }
> > +}
> > +
> >   static void colo_send_message(QEMUFile *f, COLOMessage msg,
> >                                 Error **errp)
> >   {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index f3e9bfc..f67b0a3 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4676,6 +4676,25 @@
> >   { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
> >   
> >   ##
> > +# @xen-set-replication
> > +#
> > +# Enable or disable replication
> > +#
> > +# @enable: true to enable, false to disable.
> > +#
> > +# @primary: true for primary or false for secondary
> > +#
> > +# @failover: #optional true to do failover, false to stop.
> > +#            default value is false.
> May want to additionally document that 'failover' cannot be specified if
> 'enable' is true.

OK.

Thanks
Zhang Chen

> > +#
> > +# Returns: nothing
> > +#
> > +# Since: 2.9
> > +##
> > +{ 'command': 'xen-set-replication',
> > +  'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
> > +
> > +##
> >   # @GICCapability:
> >   #
> >   # The struct describes capability for a specific GIC (Generic

--
Thanks
zhangchen