Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
We don't allow to use x-colo capability when replication is not
configured. So, no reason to build COLO when replication is disabled,
it's unusable in this case.
Note also that the check in migrate_caps_check() is not the only
restriction: some functions in migration/colo.c will just abort if
called with not defined CONFIG_REPLICATION, for example:
migration_iteration_finish()
case MIGRATION_STATUS_COLO:
migrate_start_colo_process()
colo_process_checkpoint()
abort()
It could probably make sense to have possibility to enable COLO without
REPLICATION, but this requires deeper audit of colo & replication code,
which may be done later if needed.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Nice patch. Thanks.
@@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
static void secondary_vm_do_failover(void)
{
/* COLO needs enable block-replication */
-#ifdef CONFIG_REPLICATION
int old_state;
MigrationIncomingState *mis = migration_incoming_get_current();
Error *local_err = NULL;
@@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
if (mis->migration_incoming_co) {
qemu_coroutine_enter(mis->migration_incoming_co);
}
-#else
- abort();
-#endif
}
With only this chunks you have proved that your argument is right.
abort() is never a solution.
diff --git a/migration/options.c b/migration/options.c
index 912cbadddb..eef2bd0f16 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -171,7 +171,9 @@ Property migration_properties[] = {
DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
+#ifdef CONFIG_REPLICATION
DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+#endif
DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
@@ -209,9 +211,13 @@ bool migrate_block(void)
bool migrate_colo(void)
{
+#ifdef CONFIG_REPLICATION
MigrationState *s = migrate_get_current();
return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
+#else
+ return false;
+#endif
}
#ifdef 1
bool migrate_compress(void)
@@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
MIGRATION_CAPABILITY_RDMA_PIN_ALL,
MIGRATION_CAPABILITY_COMPRESS,
MIGRATION_CAPABILITY_XBZRLE,
+#ifdef CONFIG_REPLICATION
MIGRATION_CAPABILITY_X_COLO,
+#endif
MIGRATION_CAPABILITY_VALIDATE_UUID,
MIGRATION_CAPABILITY_ZERO_COPY_SEND);
#ifdef 2
@@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
}
#endif
-#ifndef CONFIG_REPLICATION
- if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
- error_setg(errp, "QEMU compiled without replication module"
- " can't enable COLO");
- error_append_hint(errp, "Please enable replication before COLO.\n");
- return false;
- }
-#endif
-
if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
/* This check is reasonably expensive, so only when it's being
* set the first time, also it's only the destination that needs
I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
trying to get all capabilities to this format.
void migration_shutdown(void)
{
/*
* When the QEMU main thread exit, the COLO thread
* may wait a semaphore. So, we should wakeup the
* COLO thread before migration shutdown.
*/
colo_shutdown();
......
}
+void *colo_process_incoming_thread(void *opaque)
+{
+ abort();
+}
At least print an error message?
+void colo_checkpoint_notify(void *opaque)
+{
+ abort();
+}
Another error message.
It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c
changing
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
if (migration_in_colo_state()) {
colo_checkpoint_notify(s);
}
}
To
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
colo_checkpoint_refresh(s);
}
That way we can convert it to an empty function.