Re: [PATCH v4 04/19] migration: Move migrate_allow_multifd and helpers into migration.c

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Mar 31, 2022 at 11:08:42AM -0400, Peter Xu wrote:
> This variable, along with its helpers, is used to detect whether multiple
> channel will be supported for migration.  In follow up patches, there'll be
> other capability that requires multi-channels.  Hence move it outside multifd
> specific code and make it public.  Meanwhile rename it from "multifd" to
> "multi_channels" to show its real meaning.

FWIW, I would generally suggest separating the rename from the code
movement into distinct patches.

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 22 +++++++++++++++++-----
>  migration/migration.h |  3 +++
>  migration/multifd.c   | 19 ++++---------------
>  migration/multifd.h   |  2 --
>  4 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 281d33326b..596d3d30b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -180,6 +180,18 @@ static int migration_maybe_pause(MigrationState *s,
>                                   int new_state);
>  static void migrate_fd_cancel(MigrationState *s);
>  
> +static bool migrate_allow_multi_channels = true;

This is a pre-existing thing, but I'm curious why we default this to
'true', when the first thing qemu_start_incoming_migration() and
qmp_migrate() do, is to set it to 'false' and then selectively
put it back to 'true'.


>  static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
>  {
>      uintptr_t a = (uintptr_t) ap, b = (uintptr_t) bp;
> @@ -469,12 +481,12 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
>  
> -    migrate_protocol_allow_multifd(false); /* reset it anyway */
> +    migrate_protocol_allow_multi_channels(false); /* reset it anyway */
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
>          strstart(uri, "vsock:", NULL)) {
> -        migrate_protocol_allow_multifd(true);
> +        migrate_protocol_allow_multi_channels(true);
>          socket_start_incoming_migration(p ? p : uri, errp);



> @@ -2324,11 +2336,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          }
>      }
>  
> -    migrate_protocol_allow_multifd(false);
> +    migrate_protocol_allow_multi_channels(false);
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
>          strstart(uri, "vsock:", NULL)) {
> -        migrate_protocol_allow_multifd(true);
> +        migrate_protocol_allow_multi_channels(true);
>          socket_start_outgoing_migration(s, p ? p : uri, &local_err);
>  #ifdef CONFIG_RDMA
>      } else if (strstart(uri, "rdma:", &p)) {

Regardless of comments above

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|