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

[Juan Quintela <quintela@redhat.com>]

"Zhang, Chen" <chen.zhang@intel.com> wrote:
>> -----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?

My understanding of colo is pretty limited, but my mind explodes just
trying to think how many things we would have to check/do to do a
migration while in colo state.

So I guess you are right that we can't be in both at the same time.

Later, Juan.