Re: [PATCH QEMU HCI-8.1 v6 04/34] migration/cpr-exec: require a file or exec migration URI

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: cpr-exec URI check also rejects incoming socket/fd/rdma URIs, undocumented
--

commit 77db773c747012b4f8980124448a08cb0f5cd2ed
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr-exec: require a file or exec migration URI

This commit makes migration_channels_and_transport_compatible() refuse
cpr-exec mode unless the migration URI is a file or exec type. It also
updates CPR.rst to say that other URI types are refused.

> diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
> --- a/docs/devel/migration/CPR.rst
> +++ b/docs/devel/migration/CPR.rst

[ ... ]

> @@ -365,10 +365,10 @@ The VM must be started with the ``-machine aux-ram-share=on`` option.
>  Outgoing:
>    * Set the migration mode parameter to ``cpr-exec``.
>    * Set the ``cpr-exec-command`` parameter.
> -  * Issue the ``migrate`` command.  It is recommended that the URI be
> -    a ``file`` type, but one can use other types such as ``exec``,
> -    provided the command captures all the data from the outgoing side,
> -    and provides all the data to the incoming side.
> +  * Issue the ``migrate`` command.  The URI must be a ``file`` type,
> +    or an ``exec`` type, provided the command captures all the data
> +    from the outgoing side, and provides all the data to the incoming
> +    side.  Other types are refused.
>  
>  Incoming:
>    * You do not need to explicitly start new QEMU.  It is started as

[Severity: Low]
The restriction is documented only under Outgoing, but the check is also
applied on the incoming side.  Should the Incoming section mention it as
well?

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

[ ... ]

> @@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
>          return false;
>      }
>  
> +    if (migrate_mode() == MIG_MODE_CPR_EXEC &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
> +        error_setg(errp,
> +                   "cpr-exec mode requires a file or exec migration URI");
> +        return false;
> +    }
> +
>      return true;
>  }

[Severity: Low]
Is it intended that this check also applies to incoming migration?  The
commit message, the CPR.rst text and the error string describe only the
outgoing side.

migration_channels_and_transport_compatible() is shared with the incoming
path:

qemu_start_incoming_migration()
  migration_transport_compatible()
    migration_channels_and_transport_compatible()

qmp_migrate_incoming() reaches the same helper.

migrate_mode() returns cpr_get_incoming_mode() first, so in the exec'd
QEMU, after cpr_state_load() has set the incoming mode, it returns
cpr-exec:

MigMode migrate_mode(void)
{
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

When the mode is cpr-exec, any incoming URI that is not file or exec is
now refused.  That includes tcp, unix, vsock, rdma and fd:.  An fd: URI
always parses as MIGRATION_ADDRESS_TYPE_SOCKET, even when the descriptor
refers to a regular file.

A plain destination with the mode parameter set to cpr-exec is affected as
well.

Could the commit message and documentation describe the incoming-side
behavior?  Alternatively, should the check be limited to the outgoing path?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=4