Re: [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: stale comment in qmp_migrate_finish_cb() describes a removed call
--

Commit ea2a5dd391e35be2ede79c38ea1ba67a80a4cdb1
Author: Peter Xu <peterx@redhat.com>

migration: Fix crash on second migration when cancel early

This commit changes migration_cancel() so that it calls migration_cleanup()
only when the cpr-transfer HUP gsource is still pending. It also makes
qmp_migrate_finish_cb() destroy the HUP source on entry, and it adds
assertions around the hup_source handling.

Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com

> diff --git a/migration/migration.c b/migration/migration.c
> index 81d5d4dd4c06..ab1f1641c217 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2246,11 +2251,22 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
>                                        void *opaque)
>  {
>      MigrationAddress *addr = opaque;
> +    MigrationState *s = migrate_get_current();
> +
> +    /*
> +     * Detach and release the GSource right after use.  We rely on this to
> +     * detect this small cpr-transfer window of "waiting for HUP event".
> +     */
> +    cpr_transfer_source_destroy(s);
>  
>      qmp_migrate_finish(addr, NULL);
>  
>      cpr_state_close();
> -    cpr_transfer_source_destroy(migrate_get_current());
> +
> +    /*
> +     * This is redundant as we do cpr_transfer_source_destroy() at the
> +     * entry, but it's benign; glib will just skip the detach.
> +     */
>      return G_SOURCE_REMOVE;
>  }

[Severity: Low]
This isn't a bug, but the comment placed just before return G_SOURCE_REMOVE
in qmp_migrate_finish_cb() describes code that isn't there. It says that a
redundant cpr_transfer_source_destroy() is done at entry and that glib will
skip the detach.

In this backport, the trailing cpr_transfer_source_destroy(migrate_get_current())
was removed. The only destroy call is now the one at function entry, and no
second call follows the comment.

Could the comment be dropped or reworded? It could say that the source was
already destroyed at entry, so returning G_SOURCE_REMOVE is harmless. As it
stands it may mislead readers. Runtime behaviour is correct.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=13