Re: [PATCH v7 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration

[Markus Armbruster <armbru@redhat.com>]

The subject

    migration: modified migration QAPIs to accept 'channels' argument for migration

is rather long.  Try to limit subjects to about 60 characters.  Easily
done here:

    migration: New migrate and migrate-incoming argument 'channels'

Het Gala <het.gala@nutanix.com> writes:

> MigrateChannelList allows to connect accross multiple interfaces. Added
> MigrateChannelList struct as argument to migration QAPIs.


We prefer imperative mood for describing a change in a commit message:
"Add MigrateChannelList argument ..."

> 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
> migration QAPIs future proof.

If you need to respin, please reflow the paragraph like this:

  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 migration QAPIs future proof.

Or less wordy:

  We plan to support multiple channels in the future, so multiple
  interfaces can be connected.  This is why we make the new argument a
  MigrateChannelList instead of a MigrateChannel.

> For current patchset series, have limited the size of the list to single
> element (single interface) as runtime check.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c |  16 +++--
>  migration/migration.c          |  34 ++++++++--
>  qapi/migration.json            | 109 ++++++++++++++++++++++++++++++++-
>  softmmu/vl.c                   |   2 +-
>  4 files changed, 147 insertions(+), 14 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 9885d7c9f7..5f04598202 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -423,10 +423,12 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>      const char *uri = qdict_get_str(qdict, "uri");
> +    MigrationChannelList *caps = NULL;
> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>  
> -    qmp_migrate_incoming(uri, &err);
> -
> -    hmp_handle_error(mon, err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate_incoming(uri, false, caps, &err);
> +    qapi_free_MigrationChannelList(caps);
>  }
>  
>  void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
> @@ -704,9 +706,13 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      bool resume = qdict_get_try_bool(qdict, "resume", false);
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
> +    MigrationChannelList *caps = NULL;
> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> -                false, false, true, resume, &err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate(uri, false, caps, !!blk, blk, !!inc, inc,
> +                 false, false, true, resume, &err);
> +    qapi_free_MigrationChannelList(caps);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 52e2ec3502..65272ef739 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -465,10 +465,22 @@ static bool migrate_uri_parse(const char *uri,
>      return true;
>  }
>  
> -static void qemu_start_incoming_migration(const char *uri, Error **errp)
> +static void qemu_start_incoming_migration(const char *uri, bool has_channels,
> +                                          MigrationChannelList *channels,
> +                                          Error **errp)
>  {
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    if (uri && has_channels) {
> +        error_setg(errp, "'uri' and 'channels' arguments are mutually "
> +                   "exclusive; exactly one of the two should be present in "
> +                   "'migrate-incoming' qmp command ");
> +        return;
> +    }
> +
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
> @@ -1496,7 +1508,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, bool has_channels,
> +                          MigrationChannelList *channels, Error **errp)
>  {
>      Error *local_err = NULL;
>      static bool once = true;
> @@ -1514,7 +1527,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>          return;
>      }
>  
> -    qemu_start_incoming_migration(uri, &local_err);
> +    qemu_start_incoming_migration(uri, has_channels, channels, &local_err);
>  
>      if (local_err) {
>          yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1550,7 +1563,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>       * only re-setup the migration stream and poke existing migration
>       * to continue using that newly established channel.
>       */
> -    qemu_start_incoming_migration(uri, errp);
> +    qemu_start_incoming_migration(uri, false, NULL, errp);
>  }
>  
>  void qmp_migrate_pause(Error **errp)
> @@ -1684,7 +1697,8 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>      return true;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> +void qmp_migrate(const char *uri, bool has_channels,
> +                 MigrationChannelList *channels, bool has_blk, bool blk,
>                   bool has_inc, bool inc, bool has_detach, bool detach,
>                   bool has_resume, bool resume, Error **errp)
>  {
> @@ -1693,6 +1707,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationState *s = migrate_get_current();
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    if (uri && has_channels) {
> +        error_setg(errp, "'uri' and 'channels' arguments are mutually "
> +                   "exclusive; exactly one of the two should be present in "
> +                   "'migrate' qmp command ");
> +        return;
> +    }
> +
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index b583642c2d..239affdc34 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1458,6 +1458,34 @@
>      'exec': 'MigrationExecCommand',
>      'rdma': 'InetSocketAddress' } }
>  
> +##
> +# @MigrationChannelType:
> +#
> +# The migration channel-type request options.
> +#
> +# @main: Main outbound migration channel.
> +#
> +# Since 8.1
> +##
> +{ 'enum': 'MigrationChannelType',
> +  'data': [ 'main' ] }
> +
> +##
> +# @MigrationChannel:
> +#
> +# Migration stream channel parameters.
> +#
> +# @channel-type: Channel type for transfering packet information.
> +#
> +# @addr: Migration endpoint configuration on destination interface.
> +#
> +# Since 8.1
> +##
> +{ 'struct': 'MigrationChannel',
> +  'data': {
> +      'channel-type': 'MigrationChannelType',
> +      'addr': 'MigrationAddress' } }
> +
>  ##
>  # @migrate:
>  #
> @@ -1465,6 +1493,9 @@
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
>  #
> +# @channels: list of migration stream channels with each stream
> +#     in the list connected to a destination interface endpoint.

Formatting is acceptable as is, but if you need to respin, tweak it like
this:

   # @channels: list of migration stream channels with each stream in the
   #     list connected to a destination interface endpoint.

> +#
>  # @blk: do block migration (full disk copy)
>  #
>  # @inc: incremental disk copy migration
> @@ -1489,14 +1520,50 @@
>  # 3. The user Monitor's "detach" argument is invalid in QMP and should
>  #    not be used
>  #
> +# 4. The uri argument should have the Uniform Resource Identifier of
> +#    default destination VM. This connection will be bound to default
> +#    network.
> +#
> +# 5. For now, number of migration streams is restricted to one, i.e
> +#    number of items in 'channels' list is just 1.
> +#
> +# 6. The 'uri' and 'channels' arguments are mutually exclusive;
> +#    exactly one of the two should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "socket",
> +#                                    "type": "inet",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "exec",
> +#                                    "args": [ "/bin/nc", "-p", "6000",
> +#                                              "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "rdma",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ],
> +           '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
> +           '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:
> @@ -1507,6 +1574,9 @@
>  # @uri: The Uniform Resource Identifier identifying the source or
>  #     address to listen on
>  #
> +# @channels: list of migration stream channels with each stream
> +#     in the list connected to a destination interface endpoint.

   # @channels: list of migration stream channels with each stream in the
   #     list connected to a destination interface endpoint.

Likewise.

> +#
>  # Returns: nothing on success
>  #
>  # Since: 2.3
> @@ -1522,13 +1592,46 @@
>  #
>  # 3. The uri format is the same as for -incoming
>  #
> +# 5. For now, number of migration streams is restricted to one, i.e
> +#    number of items in 'channels' list is just 1.
> +#
> +# 4. The 'uri' and 'channels' arguments are mutually exclusive;
> +#    exactly one of the two should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate-incoming",
>  #      "arguments": { "uri": "tcp::4446" } }
>  # <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "socket",
> +#                                    "type": "inet",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "exec",
> +#                                    "args": [ "/bin/nc", "-p", "6000",
> +#                                              "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "rdma",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
>  ##
> -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> +{ 'command': 'migrate-incoming',
> +             'data': {'*uri': 'str',
> +                      '*channels': [ 'MigrationChannel' ] } }
>  
>  ##
>  # @xen-save-devices-state:
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b0b96f67fa..d811f3f878 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2651,7 +2651,7 @@ void qmp_x_exit_preconfig(Error **errp)
>      if (incoming) {
>          Error *local_err = NULL;
>          if (strcmp(incoming, "defer") != 0) {
> -            qmp_migrate_incoming(incoming, &local_err);
> +            qmp_migrate_incoming(incoming, false, NULL, &local_err);
>              if (local_err) {
>                  error_reportf_err(local_err, "-incoming %s: ", incoming);
>                  exit(1);

The issues I'm pointing out don't justfy yet another respin.  But if you
need to respin the series for some other reason, plase take care of them.

Acked-by: Markus Armbruster <armbru@redhat.com>