Re: [PATCH v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Daniel P . Berrangé]

On Tue, May 16, 2023 at 11:18:16AM +0530, Het Gala wrote:
> 
> On 15/05/23 4:06 pm, Daniel P. Berrangé wrote:
> > On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
> > > MigrateChannelList ideally allows to connect accross multiple interfaces.
> > > 
> > > Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
> > > MigrateChannelList in hmp_migrate() and qmp_migrate() functions.
> > > 
> > > Future patchset series plans to include multiple MigrateChannels
> > > for multiple interfaces to be connected. That is the reason, 
> > > 'MigrateChannelList'
> > > is the preferred choice of argument over 'MigrateChannel' and making 
> > > 'migrate'
> > > QAPI future proof.
> > > 
> > > For current patch, have just limit size of MigrateChannelList to 1 
> > > element as
> > > a runtime check.
> > > 
> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > >   migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
> > >   migration/migration.c          |  17 ++++-
> > >   qapi/migration.json            |  69 +++++++++++++++++++-
> > >   3 files changed, 192 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/migration/migration-hmp-cmds.c 
> > > b/migration/migration-hmp-cmds.c
> > > index 4e9f00e7dc..f098d04542 100644
> > > --- a/migration/migration-hmp-cmds.c
> > > +++ b/migration/migration-hmp-cmds.c
> > > @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
> > >                      ms->clear_bitmap_shift);
> > >   }
> > > +static bool
> > > +migrate_channel_from_qdict(MigrateChannel **channel,
> > > +                           const QDict *qdict, Error **errp)
> > > +{
> > > +    Error *err = NULL;
> > > +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> > > +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> > > +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> > > +    const char *inet_host = qdict_get_try_str(qdict, "host");
> > > +    const char *inet_port = qdict_get_try_str(qdict, "port");
> > > +    const char *unix_path = qdict_get_try_str(qdict, "path");
> > > +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> > > +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> > > +    const char *fd = qdict_get_try_str(qdict, "str");
> > > +    QList *exec = qdict_get_qlist(qdict, "exec");
> > THis seems to be implicitly defining a huge set of extra parameters
> > for the migrate 'HMP' command, but none of it is actually defined
> > at the hmp-commands.hx
> I don't have a lot of knowledge on HMP commands. I had code changes here in
> HMP merely to to keep it compatible with QMP command as it used to call
> qmp_migrate() function.
> > Do we even need todo this ?  For HMP it is not unreasonable to just
> > keep using the URI syntax forever?
> 
> Daniel, I didn't completely understand your ask here. Are you implying that
> for the HMP wrapper on top of QMP, we should pass it as a string only to
> qmp_migrate() function ?

Yeah, that's my thought. HMP is targetted at humans and aims for
user friendliness, and does not need to have 100% parity with QMP.
For the multi-channel setup, I think that's going to be something
only mgmt apps do, unlikely humans using HMP need this.

> 
> In that case, we won't be needing migrate_channel_from_qdict() function at
> all right ? Please guide.

Yeah, I think it is redundant.

An earlier patch already added an API to convert a "char *uri" into a
MigrateChannel object. So we can keep HMP using a URI, but pass it to
QMP using the MigrateChannel struct.


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