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

[Vladimir Sementsov-Ogievskiy]

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.

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

--
Best regards,
Vladimir