Re: [Qemu-devel] [PATCH COLO-Frame v8 19/34] qmp event: Add event notification for COLO error

[zhanghailiang]

On 2015/8/29 6:13, Eric Blake wrote:
> On 07/29/2015 02:45 AM, zhanghailiang wrote:
> > If some errors happen during VM's COLO FT stage, it's import to notify the users
>
> s/import/important/
>
> > this event, Togehter with 'colo_lost_heartbeat', users can intervene in COLO's
>
> s/this event,/of this event./
> s/Togehter/Together/
>
> > failover work immediately.
> > If users don't want to get involved in COLO's failover verdict,
> > it is still necessary to notify users that we exit COLO mode.
> >
> > Cc: Markus Armbruster <address@hidden>
> > Cc: Michael Roth <address@hidden>
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > ---
> >   docs/qmp/qmp-events.txt | 16 ++++++++++++++++
> >   migration/colo.c        | 11 ++++++++++-
> >   qapi/event.json         | 15 +++++++++++++++
> >   3 files changed, 41 insertions(+), 1 deletion(-)
>
> Interface review:
>
> > +++ b/docs/qmp/qmp-events.txt
> > @@ -488,6 +488,22 @@ Example:
> >   {"timestamp": {"seconds": 1432121972, "microseconds": 744001},
> >    "event": "MIGRATION", "data": {"status": "completed"}}
> >
> > +COLO_EXIT
> > +---------
>
> This file is alphabetical prior to your patch; please insert your event
> prior to DEVICE_DELETED.
>
> > +
> > +Emitted when VM finish COLO mode due to some errors happening or
>
> s/finish/finishes/
>
> > +the request of users.
> > +
> > +Data: None.
> > +
> > + - "mode": COLO mode, 'primary' or 'secondary'
> > + - "error": Error message (json-string, optional)
> > +
> > +Example:
> > +
> > +{"timestamp": {"seconds": 2032141960, "microseconds": 417172},
> > + "event": "COLO_EXIT", "data": {"mode": "primary"}}
>
> It might also be useful to provide a machine-parseable parameter on
> whether the exit was due to an internal error vs. an external request.
> Maybe by adding 'flag':'bool', since presence or absence of 'error' is
> not as nice.

Good idea, but i think the type of 'flag' changes to 'str' maybe better, which 
can be 'error' or
'request', 'bool' is not so self-explanatory.
The original reason for adding 'error' member is to help
users to know what exactly happen, but it seems to be difficult to  exactly 
class all errors,
so i will remove it.

> > +++ b/migration/colo.c
>
> > @@ -581,7 +590,7 @@ out:
> >               error_report("Secondary VM will take over work");
> >               break;
> >           }
> > -        usleep(200*1000);
> > +        usleep(200 * 1000);
> >       }
>
> Unrelated whitespace tweak. Please squash this hunk into the patch that
> introduced the problem.

OK, will fix in next version.

> > +++ b/qapi/event.json
> > @@ -255,6 +255,21 @@
> >     'data': {'status': 'MigrationStatus'}}
> >
> >   ##
> > +# @COLO_EXIT
> > +#
> > +# Emitted when VM finish COLO mode due to some errors happening or
>
> s/finish/finishes/
>
> > +# the request of users.
> > +#
> > +# @mode: 'primary' or 'secondeary'.
>
> s/secondeary/secondary/
>
> > +#
> > +# @error:  #optional, error message. Only present on error happening.
> > +#
> > +# Since: 2.4
>
> 2.5
>
> > +##
> > +{ 'event': 'COLO_EXIT',
> > +  'data': {'mode': 'str', '*error':'str'}}
>
> Please use 'mode':'COLOMode' (we already have an enum; for a finite set
> of strings, it's nicer to use the enum than the open-coded 'str').

I will fix all the above typos and other problems in next version, thanks very 
much.