Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION
Vladimir Sementsov-Ogievskiy 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 > Acked-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela
Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION
On 02.05.23 19:41, Peter Xu wrote:
##
# @query-colo-status:
@@ -1674,7 +1676,8 @@
# Since: 3.1
##
{ 'command': 'query-colo-status',
- 'returns': 'COLOStatus' }
+ 'returns': 'COLOStatus',
+ 'if': 'CONFIG_REPLICATION' }
I still see a bunch of other colo related definitions around in the qapi
schema, e.g. COLOExitReason. Is it intended to leave them as is?
Removing them will make us to add more and more ifdefs in the code.
We decided to keep x-colo capability.
One more enum field is colo migration status - not worse than x-colo enum
field, and is not possible with replication disabled
The others (like COLOExitReason) are just structure definitions - not a public
API, so no reason to care about.
The exclustion is COLOStatus, we have to handle it. If not compilation fails:
qapi/qapi-commands-migration.c:821:13: error: ‘qmp_marshal_output_COLOStatus’
defined but not used [-Werror=unused-function]
821 | static void qmp_marshal_output_COLOStatus(COLOStatus *ret_in,
| ^
cc1: all warnings being treated as errors
- COLOStatus becomes unused with replication disabled, as it is used only in
migration/colo.c
--
Best regards,
Vladimir
Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION
On Fri, Apr 28, 2023 at 10:49:21PM +0300, Vladimir Sementsov-Ogievskiy 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
> Acked-by: Dr. David Alan Gilbert
> ---
> hmp-commands.hx| 2 ++
> migration/colo.c | 28
> migration/meson.build | 6 --
> migration/migration-hmp-cmds.c | 2 ++
> migration/migration.c | 6 ++
> qapi/migration.json| 9 +---
> stubs/colo.c | 39 ++
> stubs/meson.build | 1 +
> 8 files changed, 60 insertions(+), 33 deletions(-)
> create mode 100644 stubs/colo.c
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
>migration (or once already in postcopy).
> ERST
>
> +#ifdef CONFIG_REPLICATION
> {
> .name = "x_colo_lost_heartbeat",
> .args_type = "",
> @@ -1043,6 +1044,7 @@ ERST
>"a failover or takeover is needed.",
> .cmd = hmp_x_colo_lost_heartbeat,
> },
> +#endif
>
> SRST
> ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c
> index c9e0b909b9..6c7c313956 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -26,9 +26,7 @@
> #include "qemu/rcu.h"
> #include "migration/failover.h"
> #include "migration/ram.h"
> -#ifdef CONFIG_REPLICATION
> #include "block/replication.h"
> -#endif
> #include "net/colo-compare.h"
> #include "net/colo.h"
> #include "block/block.h"
> @@ -86,7 +84,6 @@ void colo_checkpoint_delay_set(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;
> @@ -151,14 +148,10 @@ static void secondary_vm_do_failover(void)
> if (mis->migration_incoming_co) {
> qemu_coroutine_enter(mis->migration_incoming_co);
> }
> -#else
> -abort();
> -#endif
> }
>
> static void primary_vm_do_failover(void)
> {
> -#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
> int old_state;
> Error *local_err = NULL;
> @@ -199,9 +192,6 @@ static void primary_vm_do_failover(void)
>
> /* Notify COLO thread that failover work is finished */
> qemu_sem_post(>colo_exit_sem);
> -#else
> -abort();
> -#endif
> }
>
> COLOMode get_colo_mode(void)
> @@ -235,7 +225,6 @@ void colo_do_failover(void)
> }
> }
>
> -#ifdef CONFIG_REPLICATION
> void qmp_xen_set_replication(bool enable, bool primary,
> bool has_failover, bool failover,
> Error **errp)
> @@ -289,7 +278,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
> /* Notify all filters of all NIC to do checkpoint */
> colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
> }
> -#endif
>
> COLOStatus *qmp_query_colo_status(Error **errp)
> {
> @@ -453,15 +441,11 @@ static int
> colo_do_checkpoint_transaction(MigrationState *s,
> }
> qemu_mutex_lock_iothread();
>
> -#ifdef CONFIG_REPLICATION
> replication_do_checkpoint_all(_err);
> if (local_err) {
> qemu_mutex_unlock_iothread();
> goto out;
> }
> -#else
> -abort();
> -#endif
>
> colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, _err);
> if (local_err) {
> @@ -579,15 +563,11 @@ static void colo_process_checkpoint(MigrationState *s)
> object_unref(OBJECT(bioc));
>
> qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
> replication_start_all(REPLICATION_MODE_PRIMARY, _err);
> if (local_err) {
> qemu_mutex_unlock_iothread();
> goto out;
> }
> -#else
> -abort();
> -#endif
>
> vm_start();
> qemu_mutex_unlock_iothread();
> @@ -755,7 +735,6 @@ static void
> colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> return;
> }
>
> -#ifdef CONFIG_REPLICATION
> replication_get_error_all(_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@
[PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION
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
Acked-by: Dr. David Alan Gilbert
---
hmp-commands.hx| 2 ++
migration/colo.c | 28
migration/meson.build | 6 --
migration/migration-hmp-cmds.c | 2 ++
migration/migration.c | 6 ++
qapi/migration.json| 9 +---
stubs/colo.c | 39 ++
stubs/meson.build | 1 +
8 files changed, 60 insertions(+), 33 deletions(-)
create mode 100644 stubs/colo.c
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb85ee1d26..fbd0932232 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1035,6 +1035,7 @@ SRST
migration (or once already in postcopy).
ERST
+#ifdef CONFIG_REPLICATION
{
.name = "x_colo_lost_heartbeat",
.args_type = "",
@@ -1043,6 +1044,7 @@ ERST
"a failover or takeover is needed.",
.cmd = hmp_x_colo_lost_heartbeat,
},
+#endif
SRST
``x_colo_lost_heartbeat``
diff --git a/migration/colo.c b/migration/colo.c
index c9e0b909b9..6c7c313956 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -26,9 +26,7 @@
#include "qemu/rcu.h"
#include "migration/failover.h"
#include "migration/ram.h"
-#ifdef CONFIG_REPLICATION
#include "block/replication.h"
-#endif
#include "net/colo-compare.h"
#include "net/colo.h"
#include "block/block.h"
@@ -86,7 +84,6 @@ void colo_checkpoint_delay_set(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;
@@ -151,14 +148,10 @@ static void secondary_vm_do_failover(void)
if (mis->migration_incoming_co) {
qemu_coroutine_enter(mis->migration_incoming_co);
}
-#else
-abort();
-#endif
}
static void primary_vm_do_failover(void)
{
-#ifdef CONFIG_REPLICATION
MigrationState *s = migrate_get_current();
int old_state;
Error *local_err = NULL;
@@ -199,9 +192,6 @@ static void primary_vm_do_failover(void)
/* Notify COLO thread that failover work is finished */
qemu_sem_post(>colo_exit_sem);
-#else
-abort();
-#endif
}
COLOMode get_colo_mode(void)
@@ -235,7 +225,6 @@ void colo_do_failover(void)
}
}
-#ifdef CONFIG_REPLICATION
void qmp_xen_set_replication(bool enable, bool primary,
bool has_failover, bool failover,
Error **errp)
@@ -289,7 +278,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
/* Notify all filters of all NIC to do checkpoint */
colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
}
-#endif
COLOStatus *qmp_query_colo_status(Error **errp)
{
@@ -453,15 +441,11 @@ static int colo_do_checkpoint_transaction(MigrationState
*s,
}
qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
replication_do_checkpoint_all(_err);
if (local_err) {
qemu_mutex_unlock_iothread();
goto out;
}
-#else
-abort();
-#endif
colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, _err);
if (local_err) {
@@ -579,15 +563,11 @@ static void colo_process_checkpoint(MigrationState *s)
object_unref(OBJECT(bioc));
qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
replication_start_all(REPLICATION_MODE_PRIMARY, _err);
if (local_err) {
qemu_mutex_unlock_iothread();
goto out;
}
-#else
-abort();
-#endif
vm_start();
qemu_mutex_unlock_iothread();
@@ -755,7 +735,6 @@ static void
colo_incoming_process_checkpoint(MigrationIncomingState *mis,
return;
}
-#ifdef CONFIG_REPLICATION
replication_get_error_all(_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -772,9 +751,6 @@ static void
colo_incoming_process_checkpoint(MigrationIncomingState *mis,
qemu_mutex_unlock_iothread();
return;
}
-#else
-abort();
-#endif
/* Notify all filters of all NIC to do checkpoint */
colo_notify_filters_event(COLO_EVENT_CHECKPOINT, _err);
@@ -881,15 +857,11 @@ void *colo_process_incoming_thread(void *opaque)
