Re: [PATCH v2 5/8] migration: Add migration_capabilities_and_transport_compatible() helper

[Peter Xu <peterx@redhat.com>]

On Tue, Feb 25, 2025 at 06:37:21AM +0000, Zhijian Li (Fujitsu) wrote:
> 
> 
> On 25/02/2025 03:58, Peter Xu wrote:
> > On Fri, Feb 21, 2025 at 02:36:09PM +0800, Li Zhijian wrote:
> >> Similar to migration_channels_and_transport_compatible(), introduce a
> >> new helper migration_capabilities_and_transport_compatible() to check if
> >> the capabilites is compatible with the transport.
> >>
> >> Currently, only move the capabilities vs RDMA transport to this
> >> function.
> >>
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> > 
> > Yeah this is even better, thanks.
> > 
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Hi Peter,
> 
> Thinking one step further, this patch looks promising and can check
> most situations. However, on the destination side, if the user first
> specifies '-incoming' (with the startup parameter -incoming xxx or
> migrate_incoming xxx) and then 'migrate_set_capability xxx on',
> the function migration_capabilities_and_transport_compatible() will
> not be called to check compatibility, which might lead to migration failure.
> 
> To address this, without introducing a new member 'transport' into the MigrationState
> structure, the code might need to be adjusted to this:
> 
> The question is whether we need to consider it now(in this patch set)?

We can do that in one patch.

> 
>   static bool migration_transport_compatible(MigrationAddress *addr, Error **errp)
>   {
>       return migration_channels_and_transport_compatible(addr, errp) &&
> -           migration_capabilities_and_transport_compatible(addr, errp);
> +           migration_capabilities_and_transport_compatible(addr->transport,
> +               migrate_get_current()->capabilities, errp);

Here IMHO we could make migration_capabilities_and_transport_compatible()
taking addr+errp like before, then..

>   }
> 
>   static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
> diff --git a/migration/options.c b/migration/options.c
> index bb259d192a9..59f0ed5b528 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -439,6 +439,29 @@ static bool migrate_incoming_started(void)
>       return !!migration_incoming_get_current()->transport_data;
>   }
>   
> +bool
> +migration_capabilities_and_transport_compatible(MigrationAddressType transport,
> +                                                bool *new_caps,
> +                                                Error **errp)
> +{

..  here fetch global capability list and feed it.

> +    if (transport == MIGRATION_ADDRESS_TYPE_RDMA) {

[1]

> +        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
> +            error_setg(errp, "RDMA and XBZRLE can't be used together");
> +            return false;
> +        }
> +        if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
> +            error_setg(errp, "RDMA and multifd can't be used together");
> +            return false;
> +        }
> +        if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> +            error_setg(errp, "RDMA and postcopy-ram can't be used together");
> +            return false;
> +        }

We could introduce migration_rdma_caps_check(&caps, errp) for this chunk
(since [1]), then...

> +    }
> +
> +    return true;
> +}
> +
>   /**
>    * @migration_caps_check - check capability compatibility
>    *
> @@ -602,6 +625,15 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>           }
>       }
>   
> +    /*
> +     * In destination side, check the cases that capability is being set
> +     * after incoming thread has started.
> +    */
> +    if (migrate_rdma() &&
> +        !migration_capabilities_and_transport_compatible(
> +            MIGRATION_ADDRESS_TYPE_RDMA, new_caps, errp)) {
> +        return false;
> +    }

... use migration_rdma_caps_check() here, might be slightly more readable:

  if (migrate_rdma() && !migration_rdma_caps_check(new_caps, errp)) {
      return false;
  }

>       return true;
>   }
>   
> diff --git a/migration/options.h b/migration/options.h
> index 762be4e641a..ca6a40e7545 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -58,6 +58,9 @@ bool migrate_tls(void);
>   /* capabilities helpers */
>   
>   bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
> +bool
> +migration_capabilities_and_transport_compatible(MigrationAddressType transport,
> +                                                bool *new_caps, Error **errp);
> 
> > 

-- 
Peter Xu