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 <vsement...@yandex-team.ru>
Sent: Saturday, April 29, 2023 3:49 AM
To: qemu-devel@nongnu.org
Cc: lukasstra...@web.de; quint...@redhat.com; Zhang, Chen
<chen.zh...@intel.com>; vsement...@yandex-team.ru; Peter Xu
<pet...@redhat.com>; Leonardo Bras <leob...@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 <vsement...@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