[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V7 10/17] qmp event: Add COLO_EXIT event to noti
From: |
Zhang Chen |
Subject: |
Re: [Qemu-devel] [PATCH V7 10/17] qmp event: Add COLO_EXIT event to notify users while exited COLO |
Date: |
Sun, 20 May 2018 23:20:10 +0800 |
On Thu, May 17, 2018 at 4:19 PM, Markus Armbruster <address@hidden>
wrote:
> Zhang Chen <address@hidden> writes:
>
> > On Tue, May 15, 2018 at 10:29 PM, Markus Armbruster <address@hidden>
> > wrote:
> >
> >> Zhang Chen <address@hidden> writes:
> >>
> >> > From: zhanghailiang <address@hidden>
> >> >
> >> > If some errors happen during VM's COLO FT stage, it's important to
> >> > notify the users of this event. Together with 'x-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.
> >> >
> >> > Signed-off-by: zhanghailiang <address@hidden>
> >> > Signed-off-by: Li Zhijian <address@hidden>
> >> > Signed-off-by: Zhang Chen <address@hidden>
> >> > Reviewed-by: Eric Blake <address@hidden>
> >> > ---
> >> > migration/colo.c | 20 ++++++++++++++++++++
> >> > qapi/migration.json | 37 +++++++++++++++++++++++++++++++++++++
> >> > 2 files changed, 57 insertions(+)
> >> >
> >> > diff --git a/migration/colo.c b/migration/colo.c
> >> > index c083d36..8ca6381 100644
> >> > --- a/migration/colo.c
> >> > +++ b/migration/colo.c
> >> > @@ -28,6 +28,7 @@
> >> > #include "net/colo-compare.h"
> >> > #include "net/colo.h"
> >> > #include "block/block.h"
> >> > +#include "qapi/qapi-events-migration.h"
> >> >
> >> > static bool vmstate_loading;
> >> > static Notifier packets_compare_notifier;
> >> > @@ -514,6 +515,18 @@ out:
> >> > qemu_fclose(fb);
> >> > }
> >> >
> >> > + /*
> >> > + * There are only two reasons we can go here, some error
> happened.
> >> > + * Or the user triggered failover.
> >> > + */
> >> > + if (failover_get_state() == FAILOVER_STATUS_NONE) {
> >> > + qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> >> > + COLO_EXIT_REASON_ERROR, NULL);
> >> > + } else {
> >> > + qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> >> > + COLO_EXIT_REASON_REQUEST, NULL);
> >> > + }
> >>
> >> Your comment makes me suspect failover_get_state() can only be
> >> FAILOVER_STATUS_NONE or FAILOVER_STATUS_REQUIRE here. Is that correct?
> >>
> >> If yes, I recommend to add a suitable assertion.
>
> ... to make the possible states immediately obvious. The fact that you
> felt a need for a comment is further evidence of non-obviousness.
>
> >
> > Yes, and what kinds of 'suitable assertion'? Just for the
> > 'failover_get_state()' ?
>
> Here's one way to skin this cat:
>
> failover_state = failover_get_state();
> if (failover_state == FAILOVER_STATUS_NONE) {
> qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> COLO_EXIT_REASON_ERROR, NULL);
> } else {
> assert(failover_state == FAILOVER_STATUS_REQUIRE);
> qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> COLO_EXIT_REASON_REQUEST, NULL);
> }
>
> Another one:
>
> switch (failover_get_state() {
> case FAILOVER_STATUS_NONE:
> qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> COLO_EXIT_REASON_ERROR, NULL);
> break;
> case FAILOVER_STATUS_REQUIRE:
> qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
> COLO_EXIT_REASON_REQUEST, NULL);
> break;
> default:
> abort();
> }
>
> Either way, the possible states are immediately obvious. The run time
> check is a nice bonus.
>
> With just your comment, the reader still has to make the connection from
> the comment's prose to states, i.e. from "some error happened" to
> FAILOVER_STATUS_NONE, and from "user triggered failover" to
> FAILOVER_STATUS_REQUIRE.
>
> [...]
>
I got it, thanks for your detailed reply.
I will fix it in next version.
Thanks
Zhang Chen
- [Qemu-devel] [PATCH V7 14/17] filter: Add handle_event method for NetFilterClass, (continued)
- [Qemu-devel] [PATCH V7 14/17] filter: Add handle_event method for NetFilterClass, Zhang Chen, 2018/05/14
- [Qemu-devel] [PATCH V7 11/17] qapi: Add new command to query colo status, Zhang Chen, 2018/05/14
- [Qemu-devel] [PATCH V7 15/17] filter-rewriter: handle checkpoint and failover event, Zhang Chen, 2018/05/14
- [Qemu-devel] [PATCH V7 08/17] ram/COLO: Record the dirty pages that SVM received, Zhang Chen, 2018/05/14
- [Qemu-devel] [PATCH V7 10/17] qmp event: Add COLO_EXIT event to notify users while exited COLO, Zhang Chen, 2018/05/14
[Qemu-devel] [PATCH V7 12/17] savevm: split the process of different stages for loadvm/savevm, Zhang Chen, 2018/05/14
[Qemu-devel] [PATCH V7 17/17] COLO: quick failover process by kick COLO thread, Zhang Chen, 2018/05/14
[Qemu-devel] [PATCH V7 04/17] COLO: integrate colo compare with colo frame, Zhang Chen, 2018/05/14
[Qemu-devel] [PATCH V7 16/17] COLO: notify net filters about checkpoint/failover event, Zhang Chen, 2018/05/14
Re: [Qemu-devel] [PATCH V7 00/17] COLO: integrate colo frame with block replication and COLO proxy, no-reply, 2018/05/14