Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Vladimir Sementsov-Ogievskiy]

On 05.05.23 01:10, Lukas Straub wrote:
> On Fri, 28 Apr 2023 22:49:28 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>
> > We generally require same set of capabilities on source and target.
> > Let's require x-colo capability to use COLO on target.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Good patch, this is needed anyway for COLO with multifd.
>
> Also, it allows to remove a some code, like
> migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc.
> I will send patches for that.

Great! But with such changes we should be careful to not break compatibility 
between versions.. On the other hand, x-colo - is still experimental with that 
x-, so there is no guarantee how it works.

> Or you can do it if you like.

To be honest, I don't :)

> Please update the docs like below, then:
> Reviewed-by: Lukas Straub <lukasstraub2@web.de>
>
> diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt
> index 8ec653f81c..2e760a4aee 100644
> --- a/docs/COLO-FT.txt
> +++ b/docs/COLO-FT.txt
> @@ -210,6 +210,7 @@ children.0=childs0 \
>   
>   3. On Secondary VM's QEMU monitor, issue command
>   {"execute":"qmp_capabilities"}
> +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": 
> "x-colo", "state": true } ] } }
>   {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": 
> "0.0.0.0", "port": "9999"} } } }
>   {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": 
> true } }

I'll resend with this addition, thanks for reviewing!

> > ---
> >   migration/migration.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 8c5bbf3e94..5e162c0622 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
> >       return -ENOTSUP;
> >   #endif
> >   
> > +    if (!migrate_colo()) {
> > +        error_report("ENABLE_COLO command come in migration stream, but c-colo 
> > "
> > +                     "capability is not set");
> > +        return -EINVAL;
> > +    }
> > +
> >       if (ram_block_discard_disable(true)) {
> >           error_report("COLO: cannot disable RAM discard");
> >           return -EBUSY;

--
Best regards,
Vladimir