RE: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, May 4, 2023 4:23 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO
> state
> 
> On 04.05.23 11:09, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Saturday, April 29, 2023 3:49 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in
> >> COLO state
> >>
> >> COLO is not listed as running state in migrate_is_running(), so, it's
> >> theoretically possible to disable colo capability in COLO state and
> >> the unexpected error in migration_iteration_finish() is reachable.
> >>
> >> Let's disallow that in qmp_migrate_set_capabilities. Than the error
> >> becomes absolutely unreachable: we can get into COLO state only with
> >> enabled capability and can't disable it while we are in COLO state.
> >> So substitute the error by simple assertion.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex-team.ru>
> >> ---
> >>   migration/migration.c | 5 +----
> >>   migration/options.c   | 2 +-
> >>   2 files changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c index
> >> 0d912ee0d7..8c5bbf3e94 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2751,10 +2751,7 @@ static void
> >> migration_iteration_finish(MigrationState *s)
> >>           runstate_set(RUN_STATE_POSTMIGRATE);
> >>           break;
> >>       case MIGRATION_STATUS_COLO:
> >> -        if (!migrate_colo()) {
> >> -            error_report("%s: critical error: calling COLO code without "
> >> -                         "COLO enabled", __func__);
> >> -        }
> >> +        assert(migrate_colo());
> >>           migrate_start_colo_process(s);
> >>           s->vm_was_running = true;
> >>           /* Fallthrough */
> >> diff --git a/migration/options.c b/migration/options.c index
> >> 865a0214d8..f461d02eeb 100644
> >> --- a/migration/options.c
> >> +++ b/migration/options.c
> >> @@ -598,7 +598,7 @@ void
> >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> >>       MigrationCapabilityStatusList *cap;
> >>       bool new_caps[MIGRATION_CAPABILITY__MAX];
> >>
> >> -    if (migration_is_running(s->state)) {
> >> +    if (migration_is_running(s->state) || migration_in_colo_state())
> >> + {
> >
> > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()"
> is a better way?
> 
> I wasn't sure that that's correct.. migration_is_running() is used in several
> places, to do so, I'd have to analyze them all.

Actually, when running in the "MIGRATION_STATUS_COLO" means QEMU can not
do a normal migration at the same time.
Juan have any comments here?

Thanks
Chen

> 
> > Like the "migration_is_setup_ot_active()".
> >
> > Thanks
> > Chen
> >
> >>           error_setg(errp, QERR_MIGRATION_ACTIVE);
> >>           return;
> >>       }
> >> --
> >> 2.34.1
> >
> 
> --
> Best regards,
> Vladimir