Re: [PATCH QEMU HCI-8.1 v4 04/31] 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 allow-list rejects fd: and leaves CPR.rst wording stale
--

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

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

This patch makes migration_channels_and_transport_compatible() reject any
migration URI in cpr-exec mode unless its transport is file or exec. The aim
is to stop a socket URI from sending a stream without guest RAM and then
exec'ing the stale cpr-exec command.

> 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;
> +    }
> +

[Severity: Low]
Can this check reject an fd: URI that was previously accepted?

An fd: URI has transport MIGRATION_ADDRESS_TYPE_SOCKET with
SOCKET_ADDRESS_TYPE_FD. It therefore fails the new check together with tcp,
unix and vsock, even when the fd is a pipe or a regular file.

fd_start_outgoing_migration() still accepts such an fd. It only warns:

    if (!migration_fd_valid(fd)) {
        warn_report("fd: migration to a file is deprecated."
                    " Use file: instead.");
    }

So "getfd NAME" followed by "migrate fd:NAME" with mode=cpr-exec now fails
in migration_channels_and_transport_compatible() with "cpr-exec mode
requires a file or exec migration URI".

It is unclear whether fd: ever worked with cpr-exec. A named monitor fd is
not among the fds that cpr_exec_preserve_fds() preserves. The new QEMU would
need a fresh getfd and "migrate_incoming fd:NAME".

Is the exclusion of fd: backed by a file or pipe intentional? If so, could
the commit message say so?

The commit message says the check allows "the two types the CPR
documentation allows". docs/devel/migration/CPR.rst, in the cpr-exec Usage
section for the outgoing side, says:

    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.

This wording is not exclusive, and it does not say whether fd: is allowed.
It still implies a looser rule than the code now enforces. Could CPR.rst be
updated to state the file or exec restriction explicitly?

>      return true;
>  }

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