On 28.04.23 10:30, Juan Quintela wrote:
Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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.
OK, let's start with your idea, interface may be removed later if we want.
diff --git a/stubs/colo.c b/stubs/colo.c
new file mode 100644
index 0000000000..f306ab45d6
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,37 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "migration/migration.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+
+void colo_shutdown(void)
+{
+ abort();
+}
This is wrong, it should be empty.
Oops right. Good catch
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?
OK
+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.
Sounds good. I can make a patch and include it to v4
+void migrate_start_colo_process(MigrationState *s)
+{
+ abort();
+}
Another case of changing the function interface?
case MIGRATION_STATUS_COLO:
if (!migrate_colo()) {
error_report("%s: critical error: calling COLO code without "
"COLO enabled", __func__);
}
I think, it could be abort()
migrate_start_colo_process(s);
The changes of functions interfaces are independent of this patch.
Still, making empty stubs is clearer than stubs with abort(). I'll try and if
changing the interface is not too much, will include it in v4.
--
Best regards,
Vladimir
