Re: [PATCH v2 5/6] migration: Modified 'migrate-incoming' QAPI and HMP side changes on the destination interface.

[Daniel P . Berrangé]

On Thu, Feb 09, 2023 at 06:44:40PM +0530, Het Gala wrote:
> 
> On 09/02/23 4:01 pm, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 09:35:59AM +0000, Het Gala wrote:
> > > 'migrate-incoming' QAPI design have been modified into well-defined
> > > MigrateChannel struct to prevent multiple encoding of uri strings on
> > > the destination side.'uri' parameter is kept for backward compatibility.
> > > 
> > > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > > Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > >   migration/migration-hmp-cmds.c |  8 +++++++-
> > >   migration/migration.c          |  3 ++-
> > >   qapi/migration.json            | 22 ++++++++++++++++++++--
> > >   softmmu/vl.c                   |  2 +-
> > >   4 files changed, 30 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/migration/migration-hmp-cmds.c 
> > > b/migration/migration-hmp-cmds.c
> > > index a9f65ded7a..ae3c5ea5b2 100644
> > > --- a/migration/migration-hmp-cmds.c
> > > +++ b/migration/migration-hmp-cmds.c
> > > @@ -500,8 +500,14 @@ void hmp_migrate_incoming(Monitor *mon, const QDict 
> > > *qdict)
> > >       Error *err = NULL;
> > >       const char *uri = qdict_get_str(qdict, "uri");
> > > -    qmp_migrate_incoming(uri, &err);
> > > +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> > > +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> > > +        error_setg(&err, "error in retrieving channel from qdict");
> > > +        return;
> > > +    }
> > > +    qmp_migrate_incoming(uri, channel, &err);
> > > +    qapi_free_MigrateChannel(channel);
> > >       hmp_handle_error(mon, err);
> > >   }
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index accbf72a18..e22ce2dd15 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -2314,7 +2314,8 @@ void migrate_del_blocker(Error *reason)
> > >       migration_blockers = g_slist_remove(migration_blockers, reason);
> > >   }
> > > -void qmp_migrate_incoming(const char *uri, Error **errp)
> > > +void qmp_migrate_incoming(const char *uri, MigrateChannel *channel,
> > > +                          Error **errp)
> > >   {
> > >       Error *local_err = NULL;
> > >       static bool once = true;
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index 79acfcfe4e..3a88912f4d 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -1623,7 +1623,11 @@
> > >   # with -incoming defer
> > >   #
> > >   # @uri: The Uniform Resource Identifier identifying the source or
> > > -#       address to listen on
> > > +#       the address to listen on
> > > +#
> > > +# @channel: Struct containing migration channel type, along with
> > > +#           all the details of the destination interface required
> > > +#           for the address to listen on for migration stream.
> > >   #
> > >   # Returns: nothing on success
> > >   #
> > > @@ -1640,14 +1644,28 @@
> > >   #
> > >   # 3. The uri format is the same as for -incoming
> > >   #
> > > +# 4. The 'uri' and 'channel' arguments are mutually exclusive but, 
> > > atleast
> > > +#    one of the two arguments should be present.
> > > +#
> > >   # Example:
> > >   #
> > >   # -> { "execute": "migrate-incoming",
> > >   #      "arguments": { "uri": "tcp::4446" } }
> > >   # <- { "return": {} }
> > >   #
> > > +# -> { "execute": "migrate-incoming",
> > > +#      "arguments": {
> > > +#          "channel": { "channeltype": "main",
> > > +#                        "addr": { "transport": "socket",
> > > +#                                  "socket-type": { "type": "inet",
> > > +#                                                   "host": "10.12.34.9",
> > > +#                                                   "port": "1050" } } } 
> > > } }
> > > +# <- { "return": {} }
> > > +#
> > >   ##
> > > -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> > > +{ 'command': 'migrate-incoming',
> > > +             'data': {'*uri': 'str',
> > > +                      '*channel': 'MigrateChannel'} }
> > Same question of whether we need to future proof now by making this
> > 
> >    '*channels': ['MigrateChannel']
> > 
> > ?
> > 
> > Otherwise we'll have to add this 'channels' field later, and
> > end up with 'channel' and 'channels' and 'uri'
> 
> Why do we need 3 fields ? We can directly convert 'channel' into 'channels'.

That isnt guaranteed to be permitted

Once QEMU is released, the API guarantee applies (unless it was
marked 'feature: unsable'.

IOW, if we release QEMU with 'channel' parameter and then the
'channels' parameter isn't merged until the next release, we're
stuck with both and have to go through the deprecation process
for 'channel'.


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