Re: [Qemu-devel] [PATCH COLO-Frame v12 25/38] qmp event: Add event notification for COLO error

[Hailiang Zhang]

On 2015/12/19 0:03, Eric Blake wrote:
> On 12/15/2015 01:22 AM, zhanghailiang wrote:
> > If some errors happen during VM's COLO FT stage, it's important to notify the 
> > users
> > of this event. Together with 'colo_lost_heartbeat', users can intervene in 
> > COLO's
> > 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 exited 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>
> > ---
> > v11:
> > - Fix several typos found by Eric
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > ---
>
> > +++ b/docs/qmp-events.txt
> > @@ -184,6 +184,23 @@ Example:
> >   Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR
> >   event.
> >
> > +COLO_EXIT
> > +---------
> > +
> > +Emitted when VM finishes COLO mode due to some errors happening or
> > +at the request of users.
> > +
> > +Data:
> > +
> > + - "mode": COLO mode, primary or secondary side (json-string)
> > + - "reason":  the exit reason, internal error or external request. 
> > (json-string)
> > + - "error": error message (json-string, operation)
>
> s/operation/optional/
> May want to word it as:
>
> - "error": error message for human consumption (json-string, optional)
>
> to point out that machines shouldn't parse it.

Good idea, i will fix it like that.

> > +++ b/migration/colo.c
> > @@ -18,6 +18,7 @@
> >   #include "qemu/error-report.h"
> >   #include "qemu/sockets.h"
> >   #include "migration/failover.h"
> > +#include "qapi-event.h"
> >
> >   /* colo buffer */
> >   #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
> > @@ -349,6 +350,11 @@ static void colo_process_checkpoint(MigrationState *s)
> >   out:
> >       if (ret < 0) {
> >           error_report("%s: %s", __func__, strerror(-ret));
>
> Unrelated: I mentioned in another thread that we may want to start
> thinking about adding error_report_errno(); this would be another client.

Hmm, yes, we may need such a helper function.

> > +++ b/qapi-schema.json
> > @@ -778,6 +778,22 @@
> >     'data': [ 'unknown', 'primary', 'secondary'] }
> >
> >   ##
> > +# @COLOExitReason
> > +#
> > +# The reason for a COLO exit
> > +#
> > +# @unknown: unknown reason
> > +#
>
> If we never return 'unknown', then it is not worth having it in the enum
> (we can always add it later if we find a reason to have it; but adding
> it now feels premature if the code base isn't using it).

You are right, it should never happen, i will remove it in next version, thanks.

> Otherwise looks okay to me.