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.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
> +bool migrate_colo_enabled(void)
> +{
> + MigrationState *s = migrate_get_current();
> + return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> +}
I consolidated all the capabilities check on my series on the list on
options.c, so this will go there.
> diff --git a/migration/migration.c b/migration/migration.c
> index bda4789193..2382958364 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -165,7 +165,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
Why?
I very much preffer the capability to be there and just fail when we try
to enable it. That way we only need the #ifdef in one place and not all
over the place.
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
> @@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
> }
> #endif
>
> -#ifndef CONFIG_REPLICATION
> - if (cap_list[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
> -
See, like this O:-)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..93863fa88c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -486,7 +486,8 @@
> { 'enum': 'MigrationCapability',
> 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> 'compress', 'events', 'postcopy-ram',
> - { 'name': 'x-colo', 'features': [ 'unstable' ] },
> + { 'name': 'x-colo', 'features': [ 'unstable' ],
> + 'if': 'CONFIG_REPLICATION' },
> 'release-ram',
> 'block', 'return-path', 'pause-before-switchover', 'multifd',
> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
Aha, It is for this that you changed the black magic on the previous
patch. Looks ok from my ignorance. As said before, I would not remove
the capability, left it the way it was.
You have less code (less #ifdefs that you just had to add), and
enabling/disabling checking capabilities don't need anything from replication.
> #
> ##
> { 'command': 'x-colo-lost-heartbeat',
> - 'features': [ 'unstable' ] }
> + 'features': [ 'unstable' ],
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @migrate_cancel:
> @@ -1685,7 +1687,8 @@
> ##
> { 'struct': 'COLOStatus',
> 'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> - 'reason': 'COLOExitReason' } }
> + 'reason': 'COLOExitReason' },
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @query-colo-status:
> @@ -1702,7 +1705,8 @@
> # Since: 3.1
> ##
> { 'command': 'query-colo-status',
> - 'returns': 'COLOStatus' }
> + 'returns': 'COLOStatus',
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @migrate-recover:
Disabling the command is ok for me.
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..45c8ac0cc6
...
> +bool migrate_colo_enabled(void)
> +{
> + return false;
> +}
You don't need this one if you follow my idea.
Notice that I fully agree with being able to disable colo O:-)
Later, Juan.
