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

[Peter Xu]

On Wed, Apr 20, 2022 at 11:41:30AM +0100, Daniel P. Berrangé wrote:
> 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.

Okay.  To still cherish Dave's R-b, I'll try to keep as-is this time, but
I'll remember it next time.

> 
> > 
> > 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'.

Agreed, FWICT it's not needed, it just doesn't hurt either.

> 
> 
> >  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>

Thanks,

-- 
Peter Xu