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

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 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];
>>> +}

>> 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?

I preffer the regularity that all capabilities are the same, and the
only place to look if something is disabled is a single place.

But on the other hand, the main user is libvirt, so

Daniel, what does libvirt preffers?

/me guess that they have to do both anyways to detect old versions, but
who knows.

Later, Juan.