Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Juan Quintela <quintela@redhat.com>]

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.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.