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

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 13:03, Juan Quintela wrote:
> 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.

Yes, I had a sense of doubt while adding these #ifdefs.

Still, on the other hand I feel that it's strange to have public interface which only can 
say "I'm not built in" :)

Actually with my way, we have just two #ifdefs instead of one. Seems, not too many. And 
instead of "I'm not supported" error we just not include the public interface 
for unsupported feature. It seems to be better user experience. What do you think?


--
Best regards,
Vladimir