Re: [PATCH QEMU HCI-8.1 v4 22/31] migration/cpr: add cpr_is_in_progress()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration/cpr: cpr_is_in_progress() false on cpr-transfer source after HUP
--

commit 6f0604877a0fdc026e4268cdb035a9a9897fd0b4
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr: add cpr_is_in_progress()

This adds cpr_is_in_progress(). It returns true on the target until the
incoming migration is done. On the source it returns true only while the
CPR state file is open.

> diff --git a/migration/cpr.c b/migration/cpr.c
> --- a/migration/cpr.c
> +++ b/migration/cpr.c

[ ... ]

> +bool cpr_is_in_progress(void)
> +{
> +    /* The incoming mode on the target, the 'mode' parameter on the source */
> +    MigMode mode = migrate_mode();
> +
> +    if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
> +        return false;
> +    }
> +
> +    /* Target: the incoming mode is set until the incoming migration is done */
> +    if (cpr_is_incoming()) {
> +        return true;
> +    }
> +
> +    /*
> +     * Source: the parameter alone means nothing, it may just have been set.
> +     * An outgoing CPR migration has the CPR state open, from
> +     * cpr_state_save() to cpr_state_close().
> +     */
> +    return cpr_state_file != NULL;

[Severity: Medium]
Can this return false on the source for almost the whole outgoing
migration in cpr-transfer mode?

The source-side check follows the lifetime of cpr_state_file, not the
lifetime of the outgoing migration. In cpr-transfer mode the file is
closed early:

qmp_migrate()
  cpr_state_save()                 /* sets cpr_state_file */
  cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

The target then closes the CPR socket, and the source runs:

qmp_migrate_finish_cb()
    ...
    qmp_migrate_finish(addr, NULL);   /* only starts the outgoing connection */

    cpr_state_close();                /* cpr_state_file = NULL */

After this point the RAM transfer, vm_stop, device state save and
completion all still run. During that time migrate_mode() is
MIG_MODE_CPR_TRANSFER, cpr_is_incoming() is false on the source, and
cpr_state_file is NULL. So cpr_is_in_progress() returns false.

This contradicts the commit message ("on the source only while the
outgoing one runs") and the comment above ("from cpr_state_save() to
cpr_state_close()"). The cpr-exec case is not affected, because there the
state is closed only in migration_cleanup().

The function has no callers in this patch. However, the later patches in
the series add source-side callers that run during the migration:
vfio_is_active(), the vfio VM state change handlers, save_prepare in
hw/vfio/migration.c, and the dirty-tracking start in hw/vfio/listener.c.
In cpr-transfer mode these would see false, so VFIO device state
migration, device stop/resume and dirty tracking would not be skipped.

A VM state change handler that fires at vm_stop during completion is the
use case the commit message names.

Could the source-side check use state that lasts for the whole outgoing
migration instead of cpr_state_file?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=22