Re: [Qemu-devel] [PATCH V7 10/17] qmp event: Add COLO_EXIT event to notify users while exited COLO

[Markus Armbruster]

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.

[...]