[PATCH v2 0/6] migration: Modified 'migrate' QAPI command for migration

[PATCH v2 0/6] migration: Modified 'migrate' QAPI command for migration

[Het Gala]

This is v2 patchset for modified 'migrate' QAPI design for migration
connection.

Links to previous versions:
v1: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04339.html

Thanks to Daniel, Markus and Dr. David for their valuable insights.

v1->v2 changelog:
- major improvements in restructuring wire protocol for exec and rdma transport
  backends.
- rdma argument change from [str --> InetSocketAddress]
- exec arguemnt change from [str --> list of string]
- functions are reshuffled into new files as per the recent migration changes
  in qemu.

Abstract:
---------

Current QAPI 'migrate' command design (for initiating a migration
stream) contains information regarding different migrate transport mechanism
(tcp / unix / exec), dest-host IP address, and binding port number in form of
a string. Thus the design does seem to have some design issues. Some of the
issues, stated below are:

1. Use of string URIs is a data encoding scheme within a data encoding scheme.
   QEMU code should directly be able to work with the results from QAPI,
   without resorting to do a second level of parsing (eg. socket_parse()).
2. For features / parameters related to migration, the migration tunables needs
   to be defined and updated upfront. For example, 'migrate-set-capability'
   and 'migrate-set-parameter' is required to enable multifd capability and
   multifd-number of channels respectively. Instead, 'Multifd-channels' can
   directly be represented as a single additional parameter to 'migrate'
   QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
   be used for runtime tunables that need setting after migration has already
   started.

The current patchset focuses on solving the first problem of multi-level
encoding of URIs. The patch defines 'migrate' command as a QAPI discriminated
union for the various transport backends (like socket, exec and rdma), and on
basis of transport backends, different migration parameters are defined.

(uri) string -->  (channel) Channel-type
                            Transport-type
                            Migration parameters based on transport type

-----------------------------------------------------------------------------

Het Gala (6):
  migration: moved hmp_split_at_commma() helper func to qapi-util.c file
  migration: Updated QAPI format for 'migrate' qemu monitor command
  migration: HMP side changes for modified 'migrate' QAPI design
  migration: Avoid multiple parsing of uri in migration code flow
  migration: Modified 'migrate-incoming' QAPI and HMP side changes on
    the destination interface.
  migration: Established connection for listener sockets on the dest
    interface

 include/monitor/hmp.h          |   1 -
 include/qapi/util.h            |   1 +
 migration/exec.c               |  38 ++++++--
 migration/exec.h               |   6 +-
 migration/migration-hmp-cmds.c | 113 +++++++++++++++++++++++-
 migration/migration.c          | 144 +++++++++++++++++++++++--------
 migration/rdma.c               |  39 +++------
 migration/rdma.h               |   5 +-
 migration/socket.c             |  37 ++------
 migration/socket.h             |   5 +-
 monitor/hmp-cmds.c             |  19 ----
 net/net-hmp-cmds.c             |   2 +-
 qapi/migration.json            | 153 ++++++++++++++++++++++++++++++++-
 qapi/qapi-util.c               |  19 ++++
 softmmu/vl.c                   |   2 +-
 stats/stats-hmp-cmds.c         |   2 +-
 16 files changed, 457 insertions(+), 129 deletions(-)

-- 
2.22.3

[PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Het Gala]

renamed hmp_split_at_comma() --> str_split_at_comma()
Shifted helper function to qapi-util.c file. Give external linkage, as
this function will be handy in coming commit for migration.

Minor correction:
g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)

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>
---
 include/monitor/hmp.h  |  1 -
 include/qapi/util.h    |  1 +
 monitor/hmp-cmds.c     | 19 -------------------
 net/net-hmp-cmds.c     |  2 +-
 qapi/qapi-util.c       | 19 +++++++++++++++++++
 stats/stats-hmp-cmds.c |  2 +-
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 2220f14fc9..e80848fbd0 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -19,7 +19,6 @@
 
 bool hmp_handle_error(Monitor *mon, Error *err);
 void hmp_help_cmd(Monitor *mon, const char *name);
-strList *hmp_split_at_comma(const char *str);
 
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
diff --git a/include/qapi/util.h b/include/qapi/util.h
index 81a2b13a33..6c8d8575e3 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -29,6 +29,7 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
                      Error **errp);
 
 int parse_qapi_name(const char *name, bool complete);
+struct strList *str_split_at_comma(const char *str);
 
 /*
  * For any GenericList @list, insert @element at the front.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 34bd8c67d7..9665e6e0a5 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
     return false;
 }
 
-/*
- * Split @str at comma.
- * A null @str defaults to "".
- */
-strList *hmp_split_at_comma(const char *str)
-{
-    char **split = g_strsplit(str ?: "", ",", -1);
-    strList *res = NULL;
-    strList **tail = &res;
-    int i;
-
-    for (i = 0; split[i]; i++) {
-        QAPI_LIST_APPEND(tail, split[i]);
-    }
-
-    g_free(split);
-    return res;
-}
-
 void hmp_info_name(Monitor *mon, const QDict *qdict)
 {
     NameInfo *info;
diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
index 41d326bf5f..a3c597a727 100644
--- a/net/net-hmp-cmds.c
+++ b/net/net-hmp-cmds.c
@@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
                                             migrate_announce_params());
 
     qapi_free_strList(params->interfaces);
-    params->interfaces = hmp_split_at_comma(interfaces_str);
+    params->interfaces = str_split_at_comma(interfaces_str);
     params->has_interfaces = params->interfaces != NULL;
     params->id = g_strdup(id);
     qmp_announce_self(params, NULL);
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 63596e11c5..e26b9d957b 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -152,3 +152,22 @@ int parse_qapi_name(const char *str, bool complete)
     }
     return p - str;
 }
+
+/*
+ * Split @str at comma.
+ * A null @str defaults to "".
+ */
+strList *str_split_at_comma(const char *str)
+{
+    char **split = g_strsplit(str ? str : "", ",", -1);
+    strList *res = NULL;
+    strList **tail = &res;
+    int i;
+
+    for (i = 0; split[i]; i++) {
+        QAPI_LIST_APPEND(tail, split[i]);
+    }
+
+    g_free(split);
+    return res;
+}
diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
index 531e35d128..cfee05a076 100644
--- a/stats/stats-hmp-cmds.c
+++ b/stats/stats-hmp-cmds.c
@@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
             request->provider = provider_idx;
             if (names && !g_str_equal(names, "*")) {
                 request->has_names = true;
-                request->names = hmp_split_at_comma(names);
+                request->names = str_split_at_comma(names);
             }
             QAPI_LIST_PREPEND(request_list, request);
         }
-- 
2.22.3

[PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

Existing 'migrate' QAPI design enforces transport mechanism, ip address
of destination interface and corresponding port number in the form
of a unified string 'uri' parameter for initiating a migration stream.
This scheme has a significant flaw in it - double encoding of existing
URIs to extract migration info.

The current patch maps QAPI uri design onto well defined MigrateChannel
struct. This modified QAPI helps in preventing multi-level uri
encodings ('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>
---
 qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 2 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index c84fa10e86..79acfcfe4e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1449,12 +1449,108 @@
 ##
 { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
 
+##
+# @MigrateTransport:
+#
+# The supported communication transport mechanisms for migration
+#
+# @socket: Supported communication type between two devices for migration.
+#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
+#          'fd' already
+#
+# @exec: Supported communication type to redirect migration stream into file.
+#
+# @rdma: Supported communication type to redirect rdma type migration stream.
+#
+# Since 8.0
+##
+{ 'enum': 'MigrateTransport',
+  'data': ['socket', 'exec', 'rdma'] }
+
+##
+# @MigrateSocketAddr:
+#
+# To support different type of socket.
+#
+# @socket-type: Different type of socket connections.
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateSocketAddr',
+  'data': {'socket-type': 'SocketAddress' } }
+
+##
+# @MigrateExecAddr:
+ #
+ # Since 8.0
+ ##
+{ 'struct': 'MigrateExecAddr',
+   'data' : {'data': ['str'] } }
+
+##
+# @MigrateRdmaAddr:
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateRdmaAddr',
+   'data' : {'data': 'InetSocketAddress' } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
+#
+# Since 8.0
+##
+{ 'union' : 'MigrateAddress',
+  'base' : { 'transport' : 'MigrateTransport'},
+  'discriminator' : 'transport',
+  'data' : {
+    'socket' : 'MigrateSocketAddr',
+    'exec' : 'MigrateExecAddr',
+    'rdma': 'MigrateRdmaAddr' } }
+
+##
+# @MigrateChannelType:
+#
+# The supported options for migration channel type requests
+#
+# @main: Support request for main outbound migration control channel
+#
+# Since 8.0
+##
+{ 'enum': 'MigrateChannelType',
+  'data': [ 'main'] }
+
+##
+# @MigrateChannel:
+#
+# Information regarding migration Channel-type for transferring packets,
+# source and corresponding destination interface for socket connection
+# and number of multifd channels over the interface.
+#
+# @channeltype: Name of Channel type for transfering packet information
+#
+# @addr: SocketAddress of destination interface
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateChannel',
+  'data' : {
+	'channeltype' : 'MigrateChannelType',
+	'addr' : 'MigrateAddress' } }
+
 ##
 # @migrate:
 #
 # Migrates the current running guest to another Virtual Machine.
 #
 # @uri: the Uniform Resource Identifier of the destination VM
+#       for migration thread
+#
+# @channel: Struct containing migration channel type, along with all
+#           the details of destination interface required for initiating
+#           a migration stream.
 #
 # @blk: do block migration (full disk copy)
 #
@@ -1479,15 +1575,46 @@
 # 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. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
+#    one of the two arguments should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
 # <- { "return": {} }
 #
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channel": { "channeltype": "main",
+#                        "addr": { "transport": "socket",
+#                                  "socket-type": { "type': "inet',
+#                                                   "host": "10.12.34.9",
+#                                                   "port": "1050" } } } } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channel": { "channeltype": "main",
+#                       "addr": { "transport": "exec",
+#                                 "exec": ["/bin/nc", "-U",
+#                                          "/some/sock" ] } } } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channel": { "channeltype": "main",
+#                       "addr": { "transport": "rdma",
+#                                 "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', '*channel': 'MigrateChannel', '*blk': 'bool',
+           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
 
 ##
 # @migrate-incoming:
-- 
2.22.3

[PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Het Gala]

hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
into well defined MigrateChannel struct with help of
migrate_channel_from_qdict().
hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
argument (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 | 105 ++++++++++++++++++++++++++++++++-
 migration/migration.c          |  15 ++++-
 2 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index ef25bc8929..a9f65ded7a 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -32,6 +32,101 @@
 #include "sysemu/runstate.h"
 #include "ui/qemu-spice.h"
 
+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");
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
+    MigrateChannelType channel_type;
+    MigrateTransport transport;
+    MigrateAddress *addr = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new(SocketAddress, 1);
+    SocketAddressType type;
+    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
+
+    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
+                                   channeltype, -1, &err);
+    if (channel_type < 0) {
+        goto end;
+    }
+
+    transport = qapi_enum_parse(&MigrateTransport_lookup,
+                                transport_str, -1, &err);
+    if (transport < 0) {
+        goto end;
+    }
+
+    type = qapi_enum_parse(&SocketAddressType_lookup,
+                           socketaddr_type, -1, &err);
+    if (type < 0) {
+        goto end;
+    }
+
+    addr->transport = transport;
+
+    switch (transport) {
+    case MIGRATE_TRANSPORT_SOCKET:
+        saddr->type = type;
+
+        switch (type) {
+        case SOCKET_ADDRESS_TYPE_INET:
+            saddr->u.inet.host = (char *)inet_host;
+            saddr->u.inet.port = (char *)inet_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_UNIX:
+            saddr->u.q_unix.path = (char *)unix_path;
+            break;
+        case SOCKET_ADDRESS_TYPE_VSOCK:
+            saddr->u.vsock.cid = (char *)vsock_cid;
+            saddr->u.vsock.port = (char *)vsock_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_FD:
+            saddr->u.fd.str = (char *)fd;
+            break;
+        default:
+            error_setg(errp, "%s: Unknown socket type %d",
+                       __func__, saddr->type);
+            break;
+        }
+
+        addr->u.socket.socket_type = saddr;
+        break;
+    case MIGRATE_TRANSPORT_EXEC:
+        addr->u.exec.data = (strList *)exec;
+         break;
+    case MIGRATE_TRANSPORT_RDMA:
+        isock->host = (char *)inet_host;
+        isock->port = (char *)inet_port;
+        addr->u.rdma.data = isock;
+        break;
+    default:
+        error_setg(errp, "%s: Unknown migrate transport type %d",
+                   __func__, addr->transport);
+        break;
+    }
+
+    val->channeltype = channel_type;
+    val->addr = addr;
+    *channel = val;
+    return true;
+
+end:
+    error_propagate(errp, err);
+    return false;
+}
+
+
 void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 {
     MigrationInfo *info;
@@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
+    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(uri, channel, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
+    qapi_free_MigrateChannel(channel);
     if (hmp_handle_error(mon, err)) {
         return;
     }
diff --git a/migration/migration.c b/migration/migration.c
index 7a14aa98d8..f6dd8dbb03 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
     return true;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
-                 bool has_inc, bool inc, bool has_detach, bool detach,
-                 bool has_resume, bool resume, Error **errp)
+void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
+                 bool blk, bool has_inc, bool inc, bool has_detach,
+                 bool detach, bool has_resume, bool resume, Error **errp)
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
@@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         }
     }
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (uri && channel) {
+        error_setg(errp, "uri and channels options should be"
+                          "mutually exclusive");
+        return;
+    }
+
     migrate_protocol_allow_multi_channels(false);
     if (strstart(uri, "tcp:", &p) ||
         strstart(uri, "unix:", NULL) ||
-- 
2.22.3

[PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

In existing senario, 'migrate' QAPI argument - string uri, is encoded
twice to extract migration parameters for stream connection. This is
not a good representation of migration wire protocol as it is a data
encoding scheme within a data encoding scheme. Qemu should be able to
directly work with results from QAPI without having to do a second
level parsing.
Modified 'migrate' QAPI design supports well defined MigrateChannel
struct which plays important role in avoiding double encoding
of uri strings.

qemu_uri_parsing() parses uri string (kept for backward
compatibility) and populate the MigrateChannel struct parameters.
Migration code flow for all required migration transport types -
socket, exec and rdma is modified.

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/exec.c      | 31 ++++++++++++++++--
 migration/exec.h      |  4 ++-
 migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
 migration/rdma.c      | 30 +++++------------
 migration/rdma.h      |  3 +-
 migration/socket.c    | 21 ++++--------
 migration/socket.h    |  3 +-
 7 files changed, 110 insertions(+), 57 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 375d2e1b54..4fa9819792 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -23,14 +23,39 @@
 #include "migration.h"
 #include "io/channel-command.h"
 #include "trace.h"
+#include "qapi/error.h"
 
 
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+void init_exec_array(strList *command, const char *argv[], Error **errp)
+{
+    int i = 0;
+    strList *lst;
+
+    for (lst = command; lst ; lst = lst->next) {
+        argv[i++] = lst->value;
+    }
+
+    /*
+     * Considering exec command always has 3 arguments to execute
+     * a command directly from the bash itself.
+     */
+    if (i > 3) {
+        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
+        return;
+    }
+
+    argv[i] = NULL;
+    return;
+}
+
+void exec_start_outgoing_migration(MigrationState *s, strList *command,
+                                   Error **errp)
 {
     QIOChannel *ioc;
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
+    const char *argv[4];
+    init_exec_array(command, argv, errp);
 
-    trace_migration_exec_outgoing(command);
+    trace_migration_exec_outgoing(argv[2]);
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
                                                     O_RDWR,
                                                     errp));
diff --git a/migration/exec.h b/migration/exec.h
index b210ffde7a..5b39ba6cbb 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -19,8 +19,10 @@
 
 #ifndef QEMU_MIGRATION_EXEC_H
 #define QEMU_MIGRATION_EXEC_H
+void init_exec_array(strList *command, const char *argv[], Error **errp);
+
 void exec_start_incoming_migration(const char *host_port, Error **errp);
 
-void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
+void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
                                    Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index f6dd8dbb03..accbf72a18 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -63,6 +63,7 @@
 #include "sysemu/cpus.h"
 #include "yank_functions.h"
 #include "sysemu/qtest.h"
+#include "qemu/sockets.h"
 #include "ui/qemu-spice.h"
 
 #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
@@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
                       QAPI_CLONE(SocketAddress, address));
 }
 
+static bool migrate_uri_parse(const char *uri,
+                              MigrateChannel **channel,
+                              Error **errp)
+{
+    Error *local_err = NULL;
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
+    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
+
+    if (strstart(uri, "exec:", NULL)) {
+        addrs->transport = MIGRATE_TRANSPORT_EXEC;
+        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
+    } else if (strstart(uri, "rdma:", NULL) &&
+               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
+        addrs->transport = MIGRATE_TRANSPORT_RDMA;
+        addrs->u.rdma.data = isock;
+    } else if (strstart(uri, "tcp:", NULL) ||
+                strstart(uri, "unix:", NULL) ||
+                strstart(uri, "vsock:", NULL) ||
+                strstart(uri, "fd:", NULL)) {
+        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
+        saddr = socket_parse(uri, &local_err);
+        addrs->u.socket.socket_type = saddr;
+    }
+
+    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
+    val->addr = addrs;
+    *channel = val;
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return false;
+    }
+
+    return true;
+}
+
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
@@ -2469,7 +2508,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    const char *p = NULL;
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
 
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
                          has_resume && resume, errp)) {
@@ -2490,22 +2530,29 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
         error_setg(errp, "uri and channels options should be"
                           "mutually exclusive");
         return;
+    } else if (uri && !migrate_uri_parse(uri, &channel, &local_err)) {
+        error_setg(errp, "Error parsing uri");
+        return;
     }
 
     migrate_protocol_allow_multi_channels(false);
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        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)) {
-        rdma_start_outgoing_migration(s, p, &local_err);
-#endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_outgoing_migration(s, p, &local_err);
-    } else if (strstart(uri, "fd:", &p)) {
-        fd_start_outgoing_migration(s, p, &local_err);
+    addrs = channel->addr;
+    saddr = channel->addr->u.socket.socket_type;
+    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
+        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+            migrate_protocol_allow_multi_channels(true);
+            socket_start_outgoing_migration(s, saddr, &local_err);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
+        }
+    #ifdef CONFIG_RDMA
+    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_outgoing_migration(s, addrs->u.rdma.data, &local_err);
+    #endif
+    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_outgoing_migration(s, addrs->u.exec.data, &local_err);
     } else {
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
diff --git a/migration/rdma.c b/migration/rdma.c
index 288eadc2d2..48f49add6f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -316,7 +316,6 @@ typedef struct RDMALocalBlocks {
 typedef struct RDMAContext {
     char *host;
     int port;
-    char *host_port;
 
     RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
 
@@ -2449,9 +2448,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
         rdma->channel = NULL;
     }
     g_free(rdma->host);
-    g_free(rdma->host_port);
     rdma->host = NULL;
-    rdma->host_port = NULL;
 }
 
 
@@ -2733,28 +2730,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
     rdma_return_path->is_return_path = true;
 }
 
-static void *qemu_rdma_data_init(const char *host_port, Error **errp)
+static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
 {
     RDMAContext *rdma = NULL;
-    InetSocketAddress *addr;
 
-    if (host_port) {
+    if (saddr) {
         rdma = g_new0(RDMAContext, 1);
         rdma->current_index = -1;
         rdma->current_chunk = -1;
 
-        addr = g_new(InetSocketAddress, 1);
-        if (!inet_parse(addr, host_port, NULL)) {
-            rdma->port = atoi(addr->port);
-            rdma->host = g_strdup(addr->host);
-            rdma->host_port = g_strdup(host_port);
-        } else {
-            ERROR(errp, "bad RDMA migration address '%s'", host_port);
-            g_free(rdma);
-            rdma = NULL;
-        }
-
-        qapi_free_InetSocketAddress(addr);
+        rdma->host = g_strdup(saddr->host);
+        rdma->port = atoi(saddr->port);
     }
 
     return rdma;
@@ -3354,6 +3340,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
                                             .private_data_len = sizeof(cap),
                                          };
     RDMAContext *rdma_return_path = NULL;
+    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
     struct rdma_cm_event *cm_event;
     struct ibv_context *verbs;
     int ret = -EINVAL;
@@ -4152,14 +4139,13 @@ err:
     error_propagate(errp, local_err);
     if (rdma) {
         g_free(rdma->host);
-        g_free(rdma->host_port);
     }
     g_free(rdma);
     g_free(rdma_return_path);
 }
 
 void rdma_start_outgoing_migration(void *opaque,
-                            const char *host_port, Error **errp)
+                            InetSocketAddress *addr, Error **errp)
 {
     MigrationState *s = opaque;
     RDMAContext *rdma_return_path = NULL;
@@ -4172,7 +4158,7 @@ void rdma_start_outgoing_migration(void *opaque,
         return;
     }
 
-    rdma = qemu_rdma_data_init(host_port, errp);
+    rdma = qemu_rdma_data_init(addr, errp);
     if (rdma == NULL) {
         goto err;
     }
@@ -4193,7 +4179,7 @@ void rdma_start_outgoing_migration(void *opaque,
 
     /* RDMA postcopy need a separate queue pair for return path */
     if (migrate_postcopy()) {
-        rdma_return_path = qemu_rdma_data_init(host_port, errp);
+        rdma_return_path = qemu_rdma_data_init(addr, errp);
 
         if (rdma_return_path == NULL) {
             goto return_path_err;
diff --git a/migration/rdma.h b/migration/rdma.h
index de2ba09dc5..8d9978e1a9 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -13,11 +13,12 @@
  * later.  See the COPYING file in the top-level directory.
  *
  */
+#include "io/channel-socket.h"
 
 #ifndef QEMU_MIGRATION_RDMA_H
 #define QEMU_MIGRATION_RDMA_H
 
-void rdma_start_outgoing_migration(void *opaque, const char *host_port,
+void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *addr,
                                    Error **errp);
 
 void rdma_start_incoming_migration(const char *host_port, Error **errp);
diff --git a/migration/socket.c b/migration/socket.c
index e6fdf3c5e1..c751e0bfc1 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -27,6 +27,8 @@
 #include "io/net-listener.h"
 #include "trace.h"
 #include "postcopy-ram.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
 
 struct SocketOutgoingArgs {
     SocketAddress *saddr;
@@ -107,19 +109,20 @@ out:
     object_unref(OBJECT(sioc));
 }
 
-static void
-socket_start_outgoing_migration_internal(MigrationState *s,
+void socket_start_outgoing_migration(MigrationState *s,
                                          SocketAddress *saddr,
                                          Error **errp)
 {
     QIOChannelSocket *sioc = qio_channel_socket_new();
     struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
+    SocketAddress *addr = g_new0(SocketAddress, 1);
+    addr = QAPI_CLONE(SocketAddress, saddr);
 
     data->s = s;
 
     /* in case previous migration leaked it */
     qapi_free_SocketAddress(outgoing_args.saddr);
-    outgoing_args.saddr = saddr;
+    outgoing_args.saddr = addr;
 
     if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
         data->hostname = g_strdup(saddr->u.inet.host);
@@ -134,18 +137,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
                                      NULL);
 }
 
-void socket_start_outgoing_migration(MigrationState *s,
-                                     const char *str,
-                                     Error **errp)
-{
-    Error *err = NULL;
-    SocketAddress *saddr = socket_parse(str, &err);
-    if (!err) {
-        socket_start_outgoing_migration_internal(s, saddr, &err);
-    }
-    error_propagate(errp, err);
-}
-
 static void socket_accept_incoming_migration(QIONetListener *listener,
                                              QIOChannelSocket *cioc,
                                              gpointer opaque)
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..95c9c166ec 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,6 +19,7 @@
 
 #include "io/channel.h"
 #include "io/task.h"
+#include "io/channel-socket.h"
 
 void socket_send_channel_create(QIOTaskFunc f, void *data);
 QIOChannel *socket_send_channel_create_sync(Error **errp);
@@ -26,6 +27,6 @@ int socket_send_channel_destroy(QIOChannel *send);
 
 void socket_start_incoming_migration(const char *str, Error **errp);
 
-void socket_start_outgoing_migration(MigrationState *s, const char *str,
+void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr,
                                      Error **errp);
 #endif
-- 
2.22.3

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

[Het Gala]

'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'} }
 
 ##
 # @xen-save-devices-state:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 9177d95d4e..16b8bdcf9b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2617,7 +2617,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, NULL, &local_err);
             if (local_err) {
                 error_reportf_err(local_err, "-incoming %s: ", incoming);
                 exit(1);
-- 
2.22.3

[PATCH v2 6/6] migration: Established connection for listener sockets on the dest interface

[Het Gala]

Modified 'migrate-incoming' QAPI design supports MigrateChannel parameters.
This well-defined struct replaces uri string to prevent multiple encodings.
(uri paramter is kept for backward compatibility).

socket_start_incoming_migration() has been deprecated  and
socket_start_incoming_migration_internal() name has been replaced with
socket_outgoing_migration().
qemu_uri_parsing() has been used to populate the migration parameters in
MigrateChannel struct.

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/exec.c      |  7 +++---
 migration/exec.h      |  2 +-
 migration/migration.c | 51 +++++++++++++++++++++++++++++--------------
 migration/rdma.c      |  9 +++++---
 migration/rdma.h      |  2 +-
 migration/socket.c    | 16 ++------------
 migration/socket.h    |  2 +-
 7 files changed, 50 insertions(+), 39 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 4fa9819792..8506ad7f18 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -77,12 +77,13 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
     return G_SOURCE_REMOVE;
 }
 
-void exec_start_incoming_migration(const char *command, Error **errp)
+void exec_start_incoming_migration(strList *command, Error **errp)
 {
     QIOChannel *ioc;
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
+    const char *argv[4];
+    init_exec_array(command, argv, errp);
 
-    trace_migration_exec_incoming(command);
+    trace_migration_exec_incoming(argv[2]);
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
                                                     O_RDWR,
                                                     errp));
diff --git a/migration/exec.h b/migration/exec.h
index 5b39ba6cbb..5335f7c24a 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -21,7 +21,7 @@
 #define QEMU_MIGRATION_EXEC_H
 void init_exec_array(strList *command, const char *argv[], Error **errp);
 
-void exec_start_incoming_migration(const char *host_port, Error **errp);
+void exec_start_incoming_migration(strList *host_port, Error **errp);
 
 void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
                                    Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index e22ce2dd15..639d727393 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -528,27 +528,46 @@ 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,
+                                          MigrateChannel *channel,
+                                          Error **errp)
 {
-    const char *p = NULL;
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
+
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (uri && channel) {
+        error_setg(errp, "uri and channels options should be used "
+                          "mutually exclusive");
+        return;
+    } else if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+        error_setg(errp, "Error parsing uri");
+        return;
+    }
 
     migrate_protocol_allow_multi_channels(false); /* reset it anyway */
+    addrs = channel->addr;
+    saddr = QAPI_CLONE(SocketAddress, channel->addr->u.socket.socket_type);
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        migrate_protocol_allow_multi_channels(true);
-        socket_start_incoming_migration(p ? p : uri, errp);
+    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
+        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+            migrate_protocol_allow_multi_channels(true);
+            socket_start_incoming_migration(saddr, errp);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_incoming_migration(saddr->u.fd.str, errp);
+        }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_incoming_migration(p, errp);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_incoming_migration(addrs->u.rdma.data, errp);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_incoming_migration(p, errp);
-    } else if (strstart(uri, "fd:", &p)) {
-        fd_start_incoming_migration(p, errp);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_incoming_migration(addrs->u.exec.data, errp);
     } else {
-        error_setg(errp, "unknown migration protocol: %s", uri);
+        error_setg(errp, "unknown migration protocol: %i", addrs->transport);
     }
 }
 
@@ -2333,7 +2352,7 @@ void qmp_migrate_incoming(const char *uri, MigrateChannel *channel,
         return;
     }
 
-    qemu_start_incoming_migration(uri, &local_err);
+    qemu_start_incoming_migration(uri, channel, &local_err);
 
     if (local_err) {
         yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -2369,7 +2388,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, NULL, errp);
 }
 
 void qmp_migrate_pause(Error **errp)
diff --git a/migration/rdma.c b/migration/rdma.c
index 48f49add6f..0225bbaf3c 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3356,12 +3356,15 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         goto err_rdma_dest_wait;
     }
 
+    isock->host = rdma->host;
+    isock->port = (char *)(intptr_t)rdma->port;
+
     /*
      * initialize the RDMAContext for return path for postcopy after first
      * connection request reached.
      */
     if (migrate_postcopy() && !rdma->is_return_path) {
-        rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
+        rdma_return_path = qemu_rdma_data_init(isock, NULL);
         if (rdma_return_path == NULL) {
             rdma_ack_cm_event(cm_event);
             goto err_rdma_dest_wait;
@@ -4093,7 +4096,7 @@ static void rdma_accept_incoming_migration(void *opaque)
     }
 }
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp)
+void rdma_start_incoming_migration(InetSocketAddress *addr, Error **errp)
 {
     int ret;
     RDMAContext *rdma, *rdma_return_path = NULL;
@@ -4107,7 +4110,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
         return;
     }
 
-    rdma = qemu_rdma_data_init(host_port, &local_err);
+    rdma = qemu_rdma_data_init(addr, &local_err);
     if (rdma == NULL) {
         goto err;
     }
diff --git a/migration/rdma.h b/migration/rdma.h
index 8d9978e1a9..40673287a7 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -21,6 +21,6 @@
 void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *addr,
                                    Error **errp);
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp);
+void rdma_start_incoming_migration(InetSocketAddress *addr, Error **errp);
 
 #endif
diff --git a/migration/socket.c b/migration/socket.c
index c751e0bfc1..6469d615d6 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -162,9 +162,8 @@ socket_incoming_migration_end(void *opaque)
     object_unref(OBJECT(listener));
 }
 
-static void
-socket_start_incoming_migration_internal(SocketAddress *saddr,
-                                         Error **errp)
+void socket_start_incoming_migration(SocketAddress *saddr,
+                                     Error **errp)
 {
     QIONetListener *listener = qio_net_listener_new();
     MigrationIncomingState *mis = migration_incoming_get_current();
@@ -202,14 +201,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
         qapi_free_SocketAddress(address);
     }
 }
-
-void socket_start_incoming_migration(const char *str, Error **errp)
-{
-    Error *err = NULL;
-    SocketAddress *saddr = socket_parse(str, &err);
-    if (!err) {
-        socket_start_incoming_migration_internal(saddr, &err);
-    }
-    qapi_free_SocketAddress(saddr);
-    error_propagate(errp, err);
-}
diff --git a/migration/socket.h b/migration/socket.h
index 95c9c166ec..4769a2bdf9 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -25,7 +25,7 @@ void socket_send_channel_create(QIOTaskFunc f, void *data);
 QIOChannel *socket_send_channel_create_sync(Error **errp);
 int socket_send_channel_destroy(QIOChannel *send);
 
-void socket_start_incoming_migration(const char *str, Error **errp);
+void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
 
 void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr,
                                      Error **errp);
-- 
2.22.3

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Eric Blake]

On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> Existing 'migrate' QAPI design enforces transport mechanism, ip address
> of destination interface and corresponding port number in the form
> of a unified string 'uri' parameter for initiating a migration stream.
> This scheme has a significant flaw in it - double encoding of existing
> URIs to extract migration info.
> 
> The current patch maps QAPI uri design onto well defined MigrateChannel
> struct. This modified QAPI helps in preventing multi-level uri
> encodings ('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>
> ---
>  qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..79acfcfe4e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,108 @@
>  ##
>  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>  
> +##
> +# @MigrateTransport:
> +#
> +# The supported communication transport mechanisms for migration
> +#
> +# @socket: Supported communication type between two devices for migration.
> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> +#          'fd' already
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# @rdma: Supported communication type to redirect rdma type migration stream.
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateTransport',
> +  'data': ['socket', 'exec', 'rdma'] }
> +
> +##
> +# @MigrateSocketAddr:
> +#
> +# To support different type of socket.
> +#
> +# @socket-type: Different type of socket connections.
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateSocketAddr',
> +  'data': {'socket-type': 'SocketAddress' } }

Here, you use 'socket-type',...

> +
> +##
> +# @MigrateExecAddr:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecAddr',
> +   'data' : {'data': ['str'] } }

Inconsistent on whether you have a space before :.  Most of our qapi
files prefer the layout:

'key': 'value'

that is, no space before, one space after.  It doesn't affect
correctness, but a consistent visual style is worth striving for.

> +
> +##
> +# @MigrateRdmaAddr:
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateRdmaAddr',
> +   'data' : {'data': 'InetSocketAddress' } }

...while these branches supply everything else under 'data'. Also,
while you documented @socket-type above, you did not document @data in
either of these two types.  [1]

> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union' : 'MigrateAddress',
> +  'base' : { 'transport' : 'MigrateTransport'},
> +  'discriminator' : 'transport',
> +  'data' : {
> +    'socket' : 'MigrateSocketAddr',
> +    'exec' : 'MigrateExecAddr',
> +    'rdma': 'MigrateRdmaAddr' } }

Another example of inconsistent spacing around :.

I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
is that SocketAddress is itself a discriminated union, and Markus does
not yet have the QAPI generator wired up to support one union as a
branch of another larger union?  It leads to extra nesting on the wire
[2]

> +
> +##
> +# @MigrateChannelType:
> +#
> +# The supported options for migration channel type requests
> +#
> +# @main: Support request for main outbound migration control channel
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateChannelType',
> +  'data': [ 'main'] }

A different spacing issue: most arrays in QAPI either have spaces at
both ends (as in [ 'string' ]) or neither (as in ['string']).  Here,
it looks lopsided with space at the front but not the back.

> +
> +##
> +# @MigrateChannel:
> +#
> +# Information regarding migration Channel-type for transferring packets,
> +# source and corresponding destination interface for socket connection
> +# and number of multifd channels over the interface.
> +#
> +# @channeltype: Name of Channel type for transfering packet information
> +#
> +# @addr: SocketAddress of destination interface

More than just a SocketAddress, per the discriminated union type defined above.

> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateChannel',
> +  'data' : {
> +	'channeltype' : 'MigrateChannelType',
> +	'addr' : 'MigrateAddress' } }
> +
>  ##
>  # @migrate:
>  #
>  # Migrates the current running guest to another Virtual Machine.
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
> +#       for migration thread
> +#
> +# @channel: Struct containing migration channel type, along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1479,15 +1575,46 @@
>  # 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. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
> +#    one of the two arguments should be present.

Grammar suggestion:

'uri' and 'channel' are mutually exclusive; exactly one of the two
should be present.

> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }

[2] This is the extra nesting that occurs because of the
'socket-type':'MigrateSocketAddr' above; getting rid of the nesting
would require 'socket-type':'SocketAddress', but QAPI would need to
support that first.

> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "exec",
> +#                                 "exec": ["/bin/nc", "-U",
> +#                                          "/some/sock" ] } } } }

Another lopsided spacing in [].

> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "rdma",
> +#                                 "rdma": { "host": "10.12.34.9",
> +#                                           "port": "1050" } } } } }

[1] These examples look wrong; above, the schema named the nesting as 'data', rather than 'exec' or 'rdma'.

> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>

But overall, I'm a fan of using a more type-accurate description of
the channel than the open-coded 'uri':'str'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Eric Blake]

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

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

Missing a '(since 8.0)' tag.

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

Grammar:

The 'uri' and 'channel' arguments are mutually exclusive; exactly one
of the two should be present.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 1:47 am, Eric Blake wrote:
> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter for initiating a migration stream.
>> This scheme has a significant flaw in it - double encoding of existing
>> URIs to extract migration info.
>>
>> +##
>> +# @MigrateTransport:
>> +#
>> +# The supported communication transport mechanisms for migration
>> +#
>> +# @socket: Supported communication type between two devices for migration.
>> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>> +#          'fd' already
>> +#
>> +# @exec: Supported communication type to redirect migration stream into file.
>> +#
>> +# @rdma: Supported communication type to redirect rdma type migration stream.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'enum': 'MigrateTransport',
>> +  'data': ['socket', 'exec', 'rdma'] }
>> +
>> +##
>> +# @MigrateSocketAddr:
>> +#
>> +# To support different type of socket.
>> +#
>> +# @socket-type: Different type of socket connections.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateSocketAddr',
>> +  'data': {'socket-type': 'SocketAddress' } }
> Here, you use 'socket-type',...
Yes, I wanted a suggestion here actually. Will 'data' instead of 
'socket-type' be the right fit ? It will also be consistent with exec 
and rdma if changed to 'data'.
>> +
>> +##
>> +# @MigrateExecAddr:
>> + #
>> + # Since 8.0
>> + ##
>> +{ 'struct': 'MigrateExecAddr',
>> +   'data' : {'data': ['str'] } }
> Inconsistent on whether you have a space before :.  Most of our qapi
> files prefer the layout:
>
> 'key': 'value'
>
> that is, no space before, one space after.  It doesn't affect
> correctness, but a consistent visual style is worth striving for.
Okay, thanks Eric for pointing it out. Will make sure I follow this 
going forward.
>> +
>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'data': 'InetSocketAddress' } }
> ...while these branches supply everything else under 'data'. Also,
> while you documented @socket-type above, you did not document @data in
> either of these two types.  [1]
Ack. In that case, I feel it would be better if I change from 
'socket-type' to 'data' to keep consistency among the QAPI.
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>> +{ 'union' : 'MigrateAddress',
>> +  'base' : { 'transport' : 'MigrateTransport'},
>> +  'discriminator' : 'transport',
>> +  'data' : {
>> +    'socket' : 'MigrateSocketAddr',
>> +    'exec' : 'MigrateExecAddr',
>> +    'rdma': 'MigrateRdmaAddr' } }
> Another example of inconsistent spacing around :.
>
> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
> is that SocketAddress is itself a discriminated union, and Markus does
> not yet have the QAPI generator wired up to support one union as a
> branch of another larger union?  It leads to extra nesting on the wire
> [2]
Yes Eric. I did search if it is possible for a union inside a branch of 
union. That's the reason, I had to choose this path for 'socket' and 
'rdma', and to keep consistency, I did the same with 'exec' too.
>> +
>> +##
>> +# @MigrateChannelType:
>> +#
>> +# The supported options for migration channel type requests
>> +#
>> +# @main: Support request for main outbound migration control channel
>> +#
>> +# Since 8.0
>> +##
>> +{ 'enum': 'MigrateChannelType',
>> +  'data': [ 'main'] }
> A different spacing issue: most arrays in QAPI either have spaces at
> both ends (as in [ 'string' ]) or neither (as in ['string']).  Here,
> it looks lopsided with space at the front but not the back.
Ack Eric. Thanks for pointing it out. Will take care about this small 
issues from next time.
>> +
>> +##
>> +# @MigrateChannel:
>> +#
>> +# Information regarding migration Channel-type for transferring packets,
>> +# source and corresponding destination interface for socket connection
>> +# and number of multifd channels over the interface.
>> +#
>> +# @channeltype: Name of Channel type for transfering packet information
>> +#
>> +# @addr: SocketAddress of destination interface
> More than just a SocketAddress, per the discriminated union type defined above.
Yes, infact one of the branches MigrateChannel. Ack.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateChannel',
>> +  'data' : {
>> +	'channeltype' : 'MigrateChannelType',
>> +	'addr' : 'MigrateAddress' } }
>> +
>>   ##
>>   # @migrate:
>>   #
>>   # Migrates the current running guest to another Virtual Machine.
>>   #
>>   # @uri: the Uniform Resource Identifier of the destination VM
>> +#       for migration thread
>> +#
>> +# @channel: Struct containing migration channel type, along with all
>> +#           the details of destination interface required for initiating
>> +#           a migration stream.
>>   #
>>   # @blk: do block migration (full disk copy)
>>   #
>> @@ -1479,15 +1575,46 @@
>>   # 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. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
>> +#    one of the two arguments should be present.
> Grammar suggestion:
>
> 'uri' and 'channel' are mutually exclusive; exactly one of the two
> should be present.
Ack.
>> +#
>>   # Example:
>>   #
>>   # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>   # <- { "return": {} }
>>   #
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                        "addr": { "transport": "socket",
>> +#                                  "socket-type": { "type': "inet',
>> +#                                                   "host": "10.12.34.9",
>> +#                                                   "port": "1050" } } } } }
> [2] This is the extra nesting that occurs because of the
> 'socket-type':'MigrateSocketAddr' above; getting rid of the nesting
> would require 'socket-type':'SocketAddress', but QAPI would need to
> support that first.
Yes Eric, excatly.
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "exec",
>> +#                                 "exec": ["/bin/nc", "-U",
>> +#                                          "/some/sock" ] } } } }
> Another lopsided spacing in [].
Ack.
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "rdma",
>> +#                                 "rdma": { "host": "10.12.34.9",
>> +#                                           "port": "1050" } } } } }
> [1] These examples look wrong; above, the schema named the nesting as 'data', rather than 'exec' or 'rdma'.
Yes, instead of 'rdma' or 'exec', it should be replaced by 'data' here 
in the examples. Ack.
>
>> +# <- { "return": {} }
>> +#
>>   ##
>>   { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> -           '*detach': 'bool', '*resume': 'bool' } }
>> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>
> But overall, I'm a fan of using a more type-accurate description of
> the channel than the open-coded 'uri':'str'.
Yes, 'uri':'str' is kept for backward compatibility right now. This will 
be depricated in later stages.
Regards,
Het Gala.

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

[Het Gala]

On 09/02/23 1:49 am, Eric Blake 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(-)
>> +++ 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.
> Missing a '(since 8.0)' tag.
Ack.
>>   #
>>   # 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.
> Grammar:
>
> The 'uri' and 'channel' arguments are mutually exclusive; exactly one
> of the two should be present.

Ack.

Regards,
Het Gala.

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > of destination interface and corresponding port number in the form
> > of a unified string 'uri' parameter for initiating a migration stream.
> > This scheme has a significant flaw in it - double encoding of existing
> > URIs to extract migration info.
> > 
> > The current patch maps QAPI uri design onto well defined MigrateChannel
> > struct. This modified QAPI helps in preventing multi-level uri
> > encodings ('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>
> > ---
> >  qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 129 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index c84fa10e86..79acfcfe4e 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1449,12 +1449,108 @@
> >  ##
> >  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> >  
> > +##
> > +# @MigrateTransport:
> > +#
> > +# The supported communication transport mechanisms for migration
> > +#
> > +# @socket: Supported communication type between two devices for migration.
> > +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > +#          'fd' already
> > +#
> > +# @exec: Supported communication type to redirect migration stream into file.
> > +#
> > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > +#
> > +# Since 8.0
> > +##
> > +{ 'enum': 'MigrateTransport',
> > +  'data': ['socket', 'exec', 'rdma'] }
> > +
> > +##
> > +# @MigrateSocketAddr:
> > +#
> > +# To support different type of socket.
> > +#
> > +# @socket-type: Different type of socket connections.
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateSocketAddr',
> > +  'data': {'socket-type': 'SocketAddress' } }
> 
> Here, you use 'socket-type',...
> 
> > +
> > +##
> > +# @MigrateExecAddr:
> > + #
> > + # Since 8.0
> > + ##
> > +{ 'struct': 'MigrateExecAddr',
> > +   'data' : {'data': ['str'] } }
> 
> Inconsistent on whether you have a space before :.  Most of our qapi
> files prefer the layout:
> 
> 'key': 'value'
> 
> that is, no space before, one space after.  It doesn't affect
> correctness, but a consistent visual style is worth striving for.
> 
> > +
> > +##
> > +# @MigrateRdmaAddr:
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateRdmaAddr',
> > +   'data' : {'data': 'InetSocketAddress' } }
> 
> ...while these branches supply everything else under 'data'. Also,
> while you documented @socket-type above, you did not document @data in
> either of these two types.  [1]
> 
> > +
> > +##
> > +# @MigrateAddress:
> > +#
> > +# The options available for communication transport mechanisms for migration
> > +#
> > +# Since 8.0
> > +##
> > +{ 'union' : 'MigrateAddress',
> > +  'base' : { 'transport' : 'MigrateTransport'},
> > +  'discriminator' : 'transport',
> > +  'data' : {
> > +    'socket' : 'MigrateSocketAddr',
> > +    'exec' : 'MigrateExecAddr',
> > +    'rdma': 'MigrateRdmaAddr' } }
> 
> Another example of inconsistent spacing around :.
> 
> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
> is that SocketAddress is itself a discriminated union, and Markus does
> not yet have the QAPI generator wired up to support one union as a
> branch of another larger union?  It leads to extra nesting on the wire
> [2]

I don't know the backstory on this limitation. Is it something that
is very difficult to resolve ? I think it is highly desirable to have
'socket': 'SocketAddress' here. It would be a shame to introduce this
better migration API design and then have it complicated by a possibly
short term limitation of QAPI.

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

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> Existing 'migrate' QAPI design enforces transport mechanism, ip address
> of destination interface and corresponding port number in the form
> of a unified string 'uri' parameter for initiating a migration stream.
> This scheme has a significant flaw in it - double encoding of existing
> URIs to extract migration info.
> 
> The current patch maps QAPI uri design onto well defined MigrateChannel
> struct. This modified QAPI helps in preventing multi-level uri
> encodings ('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>
> ---
>  qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..79acfcfe4e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,108 @@
>  ##
>  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>  
> +##
> +# @MigrateTransport:
> +#
> +# The supported communication transport mechanisms for migration
> +#
> +# @socket: Supported communication type between two devices for migration.
> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> +#          'fd' already
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# @rdma: Supported communication type to redirect rdma type migration stream.
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateTransport',
> +  'data': ['socket', 'exec', 'rdma'] }
> +
> +##
> +# @MigrateSocketAddr:
> +#
> +# To support different type of socket.
> +#
> +# @socket-type: Different type of socket connections.
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateSocketAddr',
> +  'data': {'socket-type': 'SocketAddress' } }

I'd really like this struct to go away, but if it must exist,
then call this field 'addr', as I think 'socket-type' is overly
verbose.

> +
> +##
> +# @MigrateExecAddr:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecAddr',
> +   'data' : {'data': ['str'] } }

Instead of having the field called 'data' lets me more
descriptive, and perhaps rename the struct too:

 { 'struct': 'MigrateCommand',
    'data' : {'args': ['str'] } }

Any thoughts on whether we should allow for setting env varibles
too ?

> +##
> +# @MigrateRdmaAddr:
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateRdmaAddr',
> +   'data' : {'data': 'InetSocketAddress' } }

InetSocketAddress is a plain struct, so I think we can use
that directly, no ?

> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union' : 'MigrateAddress',
> +  'base' : { 'transport' : 'MigrateTransport'},
> +  'discriminator' : 'transport',
> +  'data' : {
> +    'socket' : 'MigrateSocketAddr',
> +    'exec' : 'MigrateExecAddr',
> +    'rdma': 'MigrateRdmaAddr' } }

Ideally this would be

   'data' : {
     'socket' : 'SocketAddress',
     'exec' : 'MigrateCommand',
     'rdma': 'InetSocketAddress' } }

though the first SocketAddress isn't possible unless it is easy to
lift the QAPI limitation.


>  ##
>  # @migrate:
>  #
>  # Migrates the current running guest to another Virtual Machine.
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
> +#       for migration thread
> +#
> +# @channel: Struct containing migration channel type, along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1479,15 +1575,46 @@
>  # 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. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
> +#    one of the two arguments should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "exec",
> +#                                 "exec": ["/bin/nc", "-U",
> +#                                          "/some/sock" ] } } } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "rdma",
> +#                                 "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', '*channel': 'MigrateChannel', '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }

IIRC, the intention was to allow multiple channels to be set in a
follow up to this series. If so that would require adding yet another
field as an array of MigrateChannel.  Should we just go for the
array straight away, and just limit it to 1 element  as a runtime
check ? eg

  'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }


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

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

[Daniel P. Berrangé]

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'


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

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
> In existing senario, 'migrate' QAPI argument - string uri, is encoded
> twice to extract migration parameters for stream connection. This is
> not a good representation of migration wire protocol as it is a data
> encoding scheme within a data encoding scheme. Qemu should be able to
> directly work with results from QAPI without having to do a second
> level parsing.
> Modified 'migrate' QAPI design supports well defined MigrateChannel
> struct which plays important role in avoiding double encoding
> of uri strings.
> 
> qemu_uri_parsing() parses uri string (kept for backward
> compatibility) and populate the MigrateChannel struct parameters.
> Migration code flow for all required migration transport types -
> socket, exec and rdma is modified.
> 
> 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/exec.c      | 31 ++++++++++++++++--
>  migration/exec.h      |  4 ++-
>  migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
>  migration/rdma.c      | 30 +++++------------
>  migration/rdma.h      |  3 +-
>  migration/socket.c    | 21 ++++--------
>  migration/socket.h    |  3 +-
>  7 files changed, 110 insertions(+), 57 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index 375d2e1b54..4fa9819792 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -23,14 +23,39 @@
>  #include "migration.h"
>  #include "io/channel-command.h"
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void init_exec_array(strList *command, const char *argv[], Error **errp)
> +{
> +    int i = 0;
> +    strList *lst;
> +
> +    for (lst = command; lst ; lst = lst->next) {
> +        argv[i++] = lst->value;
> +    }
> +
> +    /*
> +     * Considering exec command always has 3 arguments to execute
> +     * a command directly from the bash itself.
> +     */
> +    if (i > 3) {
> +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
> +        return;
> +    }
> +
> +    argv[i] = NULL;
> +    return;
> +}
> +
> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +    const char *argv[4];
> +    init_exec_array(command, argv, errp);
>  
> -    trace_migration_exec_outgoing(command);
> +    trace_migration_exec_outgoing(argv[2]);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..5b39ba6cbb 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,8 +19,10 @@
>  
>  #ifndef QEMU_MIGRATION_EXEC_H
>  #define QEMU_MIGRATION_EXEC_H
> +void init_exec_array(strList *command, const char *argv[], Error **errp);
> +
>  void exec_start_incoming_migration(const char *host_port, Error **errp);
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
>                                     Error **errp);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index f6dd8dbb03..accbf72a18 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -63,6 +63,7 @@
>  #include "sysemu/cpus.h"
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/sockets.h"
>  #include "ui/qemu-spice.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
> @@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrateChannel **channel,
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));

Huh, what is the purpose of using 'str_split_at_comma' ? The format
of this is  "exec:<shell command>", because it is run as:

     const char *argv[] = { "/bin/sh", "-c", command, NULL };

we should not be trying to parse the bit after 'exec:' at all,
and certainly not splitting it on commas which makes no sense
for a shell command.

I would have expected something more like this:

    strList **tail = &addrs->u.exec.data;
    QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
    QAPI_LIST_APPEND(tail, g_strdup("-c"));
    QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));


> +    addrs = channel->addr;
> +    saddr = channel->addr->u.socket.socket_type;
> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> +            migrate_protocol_allow_multi_channels(true);
> +            socket_start_outgoing_migration(s, saddr, &local_err);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);

This is probably a sign that  fd_start_outgoing_migration() should
be folded into the socket_start_outgoing_migration() method, but
that's a separate cleanup from this patch.

> +        }
> +    #ifdef CONFIG_RDMA
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
> +        rdma_start_outgoing_migration(s, addrs->u.rdma.data, &local_err);
> +    #endif
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
> +        exec_start_outgoing_migration(s, addrs->u.exec.data, &local_err);
>      } else {
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);

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

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Markus Armbruster]

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

> renamed hmp_split_at_comma() --> str_split_at_comma()
> Shifted helper function to qapi-util.c file.

Not an appropriate home.

If we have to split up a string in QAPI/QMP, we screwed up.  Let me
explain.

In QAPI/QMP, data is not to be encoded in strings, it is to be
represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".

When you find yourself parsing QAPI/QMP string values, you're dealing
with a case where we violated this interface design principle.  Happens,
but the proper home for code helping to deal with this is *not* qapi/.
Simply because QAPI is not about parsing strings.

>                                              Give external linkage, as
> this function will be handy in coming commit for migration.

It already has external linkage.

> Minor correction:
> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)

This is not actually a correction :)

Omitting the second operand of a conditional expression like x ?: y is
equivalent to x ? x : y, except it evaluates x only once.

https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html

Besides, please don't mix code motion with code changes.

[...]

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 09:35:55AM +0000, Het Gala wrote:
> renamed hmp_split_at_comma() --> str_split_at_comma()
> Shifted helper function to qapi-util.c file. Give external linkage, as
> this function will be handy in coming commit for migration.
> 
> Minor correction:
> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
> 
> 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>
> ---
>  include/monitor/hmp.h  |  1 -
>  include/qapi/util.h    |  1 +
>  monitor/hmp-cmds.c     | 19 -------------------
>  net/net-hmp-cmds.c     |  2 +-
>  qapi/qapi-util.c       | 19 +++++++++++++++++++
>  stats/stats-hmp-cmds.c |  2 +-
>  6 files changed, 22 insertions(+), 22 deletions(-)

I expect this patch can be dropped, since I don't believe it is
correct to be using it in patch 2. I left comments in that other
patch with more details.

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

Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
> hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
> into well defined MigrateChannel struct with help of
> migrate_channel_from_qdict().
> hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
> argument (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 | 105 ++++++++++++++++++++++++++++++++-
>  migration/migration.c          |  15 ++++-
>  2 files changed, 116 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index ef25bc8929..a9f65ded7a 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -32,6 +32,101 @@
>  #include "sysemu/runstate.h"
>  #include "ui/qemu-spice.h"
>  
> +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");
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateChannelType channel_type;
> +    MigrateTransport transport;
> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new(SocketAddress, 1);
> +    SocketAddressType type;
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> +
> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
> +                                   channeltype, -1, &err);
> +    if (channel_type < 0) {
> +        goto end;
> +    }
> +
> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
> +                                transport_str, -1, &err);
> +    if (transport < 0) {
> +        goto end;
> +    }
> +
> +    type = qapi_enum_parse(&SocketAddressType_lookup,
> +                           socketaddr_type, -1, &err);
> +    if (type < 0) {
> +        goto end;
> +    }
> +
> +    addr->transport = transport;
> +
> +    switch (transport) {
> +    case MIGRATE_TRANSPORT_SOCKET:
> +        saddr->type = type;
> +
> +        switch (type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            saddr->u.inet.host = (char *)inet_host;
> +            saddr->u.inet.port = (char *)inet_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            saddr->u.q_unix.path = (char *)unix_path;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_VSOCK:
> +            saddr->u.vsock.cid = (char *)vsock_cid;
> +            saddr->u.vsock.port = (char *)vsock_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_FD:
> +            saddr->u.fd.str = (char *)fd;
> +            break;
> +        default:
> +            error_setg(errp, "%s: Unknown socket type %d",
> +                       __func__, saddr->type);
> +            break;
> +        }
> +
> +        addr->u.socket.socket_type = saddr;
> +        break;
> +    case MIGRATE_TRANSPORT_EXEC:
> +        addr->u.exec.data = (strList *)exec;
> +         break;
> +    case MIGRATE_TRANSPORT_RDMA:
> +        isock->host = (char *)inet_host;
> +        isock->port = (char *)inet_port;
> +        addr->u.rdma.data = isock;
> +        break;
> +    default:
> +        error_setg(errp, "%s: Unknown migrate transport type %d",
> +                   __func__, addr->transport);
> +        break;
> +    }
> +
> +    val->channeltype = channel_type;
> +    val->addr = addr;
> +    *channel = val;
> +    return true;
> +
> +end:
> +    error_propagate(errp, err);
> +    return false;
> +}
> +
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> +    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(uri, channel, !!blk, blk, !!inc, inc,
>                  false, false, true, resume, &err);
> +    qapi_free_MigrateChannel(channel);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 7a14aa98d8..f6dd8dbb03 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>      return true;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> -                 bool has_inc, bool inc, bool has_detach, bool detach,
> -                 bool has_resume, bool resume, Error **errp)
> +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
> +                 bool blk, bool has_inc, bool inc, bool has_detach,
> +                 bool detach, bool has_resume, bool resume, Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          }
>      }
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    if (uri && channel) {
> +        error_setg(errp, "uri and channels options should be"

s/should be/are/, also best to quote parameter names, eg

    error_setg(errp, 
               "'uri' and 'channels' options are mutually exclusive");

> +                          "mutually exclusive");
> +        return;
> +    }
> +

This change for qmp_migrate will need to be in patch 2.

QEMU needs to compile & pass tests successfully after each individual
patch. Currently it'll fail on patch 2, because the code generator
wil emit the new qmp_migrate API declaration signature, but the change
to the implementation signature is here in patch 3.

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

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
> In existing senario, 'migrate' QAPI argument - string uri, is encoded
> twice to extract migration parameters for stream connection. This is
> not a good representation of migration wire protocol as it is a data
> encoding scheme within a data encoding scheme. Qemu should be able to
> directly work with results from QAPI without having to do a second
> level parsing.
> Modified 'migrate' QAPI design supports well defined MigrateChannel
> struct which plays important role in avoiding double encoding
> of uri strings.
> 
> qemu_uri_parsing() parses uri string (kept for backward
> compatibility) and populate the MigrateChannel struct parameters.
> Migration code flow for all required migration transport types -
> socket, exec and rdma is modified.
> 
> 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/exec.c      | 31 ++++++++++++++++--
>  migration/exec.h      |  4 ++-
>  migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
>  migration/rdma.c      | 30 +++++------------
>  migration/rdma.h      |  3 +-
>  migration/socket.c    | 21 ++++--------
>  migration/socket.h    |  3 +-
>  7 files changed, 110 insertions(+), 57 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index 375d2e1b54..4fa9819792 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -23,14 +23,39 @@
>  #include "migration.h"
>  #include "io/channel-command.h"
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void init_exec_array(strList *command, const char *argv[], Error **errp)
> +{
> +    int i = 0;
> +    strList *lst;
> +
> +    for (lst = command; lst ; lst = lst->next) {
> +        argv[i++] = lst->value;
> +    }
> +
> +    /*
> +     * Considering exec command always has 3 arguments to execute
> +     * a command directly from the bash itself.
> +     */
> +    if (i > 3) {
> +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
> +        return;
> +    }

By the time this check fires, the for() loop above has already
done out of bounds writes on argv[].

> +
> +    argv[i] = NULL;
> +    return;
> +}
> +
> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +    const char *argv[4];
> +    init_exec_array(command, argv, errp);

If someone invokes 'migrate' with the old URI style, the
strList will be 3 elements, and thus argv[4] is safe.

If someone invokes 'migrate' with thue new MigrateChannel style,
the strList can be arbitrarily long and thus argv[4] will be
risk of overflow. 

>  
> -    trace_migration_exec_outgoing(command);
> +    trace_migration_exec_outgoing(argv[2]);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..5b39ba6cbb 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,8 +19,10 @@
>  
>  #ifndef QEMU_MIGRATION_EXEC_H
>  #define QEMU_MIGRATION_EXEC_H
> +void init_exec_array(strList *command, const char *argv[], Error **errp);
> +
>  void exec_start_incoming_migration(const char *host_port, Error **errp);
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
>                                     Error **errp);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index f6dd8dbb03..accbf72a18 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -63,6 +63,7 @@
>  #include "sysemu/cpus.h"
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/sockets.h"
>  #include "ui/qemu-spice.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
> @@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrateChannel **channel,
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
> +    } else if (strstart(uri, "rdma:", NULL) &&
> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> +        addrs->u.rdma.data = isock;
> +    } else if (strstart(uri, "tcp:", NULL) ||
> +                strstart(uri, "unix:", NULL) ||
> +                strstart(uri, "vsock:", NULL) ||
> +                strstart(uri, "fd:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> +        saddr = socket_parse(uri, &local_err);
> +        addrs->u.socket.socket_type = saddr;
> +    }
> +
> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
> +    val->addr = addrs;
> +    *channel = val;
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> @@ -2469,7 +2508,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    const char *p = NULL;
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>                           has_resume && resume, errp)) {
> @@ -2490,22 +2530,29 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>          error_setg(errp, "uri and channels options should be"
>                            "mutually exclusive");
>          return;
> +    } else if (uri && !migrate_uri_parse(uri, &channel, &local_err)) {
> +        error_setg(errp, "Error parsing uri");
> +        return;
>      }
>  
>      migrate_protocol_allow_multi_channels(false);
> -    if (strstart(uri, "tcp:", &p) ||
> -        strstart(uri, "unix:", NULL) ||
> -        strstart(uri, "vsock:", NULL)) {
> -        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)) {
> -        rdma_start_outgoing_migration(s, p, &local_err);
> -#endif
> -    } else if (strstart(uri, "exec:", &p)) {
> -        exec_start_outgoing_migration(s, p, &local_err);
> -    } else if (strstart(uri, "fd:", &p)) {
> -        fd_start_outgoing_migration(s, p, &local_err);
> +    addrs = channel->addr;
> +    saddr = channel->addr->u.socket.socket_type;
> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> +            migrate_protocol_allow_multi_channels(true);
> +            socket_start_outgoing_migration(s, saddr, &local_err);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> +        }
> +    #ifdef CONFIG_RDMA
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
> +        rdma_start_outgoing_migration(s, addrs->u.rdma.data, &local_err);
> +    #endif
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
> +        exec_start_outgoing_migration(s, addrs->u.exec.data, &local_err);
>      } else {
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 288eadc2d2..48f49add6f 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -316,7 +316,6 @@ typedef struct RDMALocalBlocks {
>  typedef struct RDMAContext {
>      char *host;
>      int port;
> -    char *host_port;
>  
>      RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
>  
> @@ -2449,9 +2448,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>          rdma->channel = NULL;
>      }
>      g_free(rdma->host);
> -    g_free(rdma->host_port);
>      rdma->host = NULL;
> -    rdma->host_port = NULL;
>  }
>  
>  
> @@ -2733,28 +2730,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
>      rdma_return_path->is_return_path = true;
>  }
>  
> -static void *qemu_rdma_data_init(const char *host_port, Error **errp)
> +static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
>  {
>      RDMAContext *rdma = NULL;
> -    InetSocketAddress *addr;
>  
> -    if (host_port) {
> +    if (saddr) {
>          rdma = g_new0(RDMAContext, 1);
>          rdma->current_index = -1;
>          rdma->current_chunk = -1;
>  
> -        addr = g_new(InetSocketAddress, 1);
> -        if (!inet_parse(addr, host_port, NULL)) {
> -            rdma->port = atoi(addr->port);
> -            rdma->host = g_strdup(addr->host);
> -            rdma->host_port = g_strdup(host_port);
> -        } else {
> -            ERROR(errp, "bad RDMA migration address '%s'", host_port);
> -            g_free(rdma);
> -            rdma = NULL;
> -        }
> -
> -        qapi_free_InetSocketAddress(addr);
> +        rdma->host = g_strdup(saddr->host);
> +        rdma->port = atoi(saddr->port);
>      }
>  
>      return rdma;
> @@ -3354,6 +3340,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>                                              .private_data_len = sizeof(cap),
>                                           };
>      RDMAContext *rdma_return_path = NULL;
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>      struct rdma_cm_event *cm_event;
>      struct ibv_context *verbs;
>      int ret = -EINVAL;
> @@ -4152,14 +4139,13 @@ err:
>      error_propagate(errp, local_err);
>      if (rdma) {
>          g_free(rdma->host);
> -        g_free(rdma->host_port);
>      }
>      g_free(rdma);
>      g_free(rdma_return_path);
>  }
>  
>  void rdma_start_outgoing_migration(void *opaque,
> -                            const char *host_port, Error **errp)
> +                            InetSocketAddress *addr, Error **errp)
>  {
>      MigrationState *s = opaque;
>      RDMAContext *rdma_return_path = NULL;
> @@ -4172,7 +4158,7 @@ void rdma_start_outgoing_migration(void *opaque,
>          return;
>      }
>  
> -    rdma = qemu_rdma_data_init(host_port, errp);
> +    rdma = qemu_rdma_data_init(addr, errp);
>      if (rdma == NULL) {
>          goto err;
>      }
> @@ -4193,7 +4179,7 @@ void rdma_start_outgoing_migration(void *opaque,
>  
>      /* RDMA postcopy need a separate queue pair for return path */
>      if (migrate_postcopy()) {
> -        rdma_return_path = qemu_rdma_data_init(host_port, errp);
> +        rdma_return_path = qemu_rdma_data_init(addr, errp);
>  
>          if (rdma_return_path == NULL) {
>              goto return_path_err;
> diff --git a/migration/rdma.h b/migration/rdma.h
> index de2ba09dc5..8d9978e1a9 100644
> --- a/migration/rdma.h
> +++ b/migration/rdma.h
> @@ -13,11 +13,12 @@
>   * later.  See the COPYING file in the top-level directory.
>   *
>   */
> +#include "io/channel-socket.h"
>  
>  #ifndef QEMU_MIGRATION_RDMA_H
>  #define QEMU_MIGRATION_RDMA_H
>  
> -void rdma_start_outgoing_migration(void *opaque, const char *host_port,
> +void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *addr,
>                                     Error **errp);
>  
>  void rdma_start_incoming_migration(const char *host_port, Error **errp);
> diff --git a/migration/socket.c b/migration/socket.c
> index e6fdf3c5e1..c751e0bfc1 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -27,6 +27,8 @@
>  #include "io/net-listener.h"
>  #include "trace.h"
>  #include "postcopy-ram.h"
> +#include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-sockets.h"
>  
>  struct SocketOutgoingArgs {
>      SocketAddress *saddr;
> @@ -107,19 +109,20 @@ out:
>      object_unref(OBJECT(sioc));
>  }
>  
> -static void
> -socket_start_outgoing_migration_internal(MigrationState *s,
> +void socket_start_outgoing_migration(MigrationState *s,
>                                           SocketAddress *saddr,
>                                           Error **errp)
>  {
>      QIOChannelSocket *sioc = qio_channel_socket_new();
>      struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> +    SocketAddress *addr = g_new0(SocketAddress, 1);
> +    addr = QAPI_CLONE(SocketAddress, saddr);
>  
>      data->s = s;
>  
>      /* in case previous migration leaked it */
>      qapi_free_SocketAddress(outgoing_args.saddr);
> -    outgoing_args.saddr = saddr;
> +    outgoing_args.saddr = addr;
>  
>      if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
>          data->hostname = g_strdup(saddr->u.inet.host);
> @@ -134,18 +137,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
>                                       NULL);
>  }
>  
> -void socket_start_outgoing_migration(MigrationState *s,
> -                                     const char *str,
> -                                     Error **errp)
> -{
> -    Error *err = NULL;
> -    SocketAddress *saddr = socket_parse(str, &err);
> -    if (!err) {
> -        socket_start_outgoing_migration_internal(s, saddr, &err);
> -    }
> -    error_propagate(errp, err);
> -}
> -
>  static void socket_accept_incoming_migration(QIONetListener *listener,
>                                               QIOChannelSocket *cioc,
>                                               gpointer opaque)
> diff --git a/migration/socket.h b/migration/socket.h
> index dc54df4e6c..95c9c166ec 100644
> --- a/migration/socket.h
> +++ b/migration/socket.h
> @@ -19,6 +19,7 @@
>  
>  #include "io/channel.h"
>  #include "io/task.h"
> +#include "io/channel-socket.h"
>  
>  void socket_send_channel_create(QIOTaskFunc f, void *data);
>  QIOChannel *socket_send_channel_create_sync(Error **errp);
> @@ -26,6 +27,6 @@ int socket_send_channel_destroy(QIOChannel *send);
>  
>  void socket_start_incoming_migration(const char *str, Error **errp);
>  
> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
> +void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr,
>                                       Error **errp);
>  #endif
> -- 
> 2.22.3
> 

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

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> renamed hmp_split_at_comma() --> str_split_at_comma()
>> Shifted helper function to qapi-util.c file.
>
> Not an appropriate home.

I don't have an opinion here.

> If we have to split up a string in QAPI/QMP, we screwed up.  Let me
> explain.
>
> In QAPI/QMP, data is not to be encoded in strings, it is to be
> represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".

In this case, we are already screwed up O:-)

the uri value in migration has to be split.
What this series does is creating a new way to express the command
(something on the lines that you describe), but we still have to
maintain the old way of doing it for some time, so we need this
function.

> When you find yourself parsing QAPI/QMP string values, you're dealing
> with a case where we violated this interface design principle.  Happens,
> but the proper home for code helping to deal with this is *not* qapi/.
> Simply because QAPI is not about parsing strings.

Ok, I drop my review-by.

See why I was asking for reviews from QAPI libvirt folks for this code O:-)

>>                                              Give external linkage, as
>> this function will be handy in coming commit for migration.
>
> It already has external linkage.
>
>> Minor correction:
>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
>
> This is not actually a correction :)
>
> Omitting the second operand of a conditional expression like x ?: y is
> equivalent to x ? x : y, except it evaluates x only once.

You are (way) more C layer than me.

Once told that, I think that what he wanted to do is making this c
standard, no gcc standard.

Once told that, I think that every C compiler worth its salt has this
extension.

> https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html
>
> Besides, please don't mix code motion with code changes.

Agreed.

Later, Juan.

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 3:53 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
>> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>>> of destination interface and corresponding port number in the form
>>> of a unified string 'uri' parameter for initiating a migration stream.
>>> This scheme has a significant flaw in it - double encoding of existing
>>> URIs to extract migration info.
>>>
>>> The current patch maps QAPI uri design onto well defined MigrateChannel
>>> struct. This modified QAPI helps in preventing multi-level uri
>>> encodings ('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>
>>> ---
>>>   qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 129 insertions(+), 2 deletions(-)
>>>
>>> +
>>> +##
>>> +# @MigrateRdmaAddr:
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'struct': 'MigrateRdmaAddr',
>>> +   'data' : {'data': 'InetSocketAddress' } }
>> ...while these branches supply everything else under 'data'. Also,
>> while you documented @socket-type above, you did not document @data in
>> either of these two types.  [1]
>>
>>> +
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for migration
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'union' : 'MigrateAddress',
>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>> +  'discriminator' : 'transport',
>>> +  'data' : {
>>> +    'socket' : 'MigrateSocketAddr',
>>> +    'exec' : 'MigrateExecAddr',
>>> +    'rdma': 'MigrateRdmaAddr' } }
>> Another example of inconsistent spacing around :.
>>
>> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
>> is that SocketAddress is itself a discriminated union, and Markus does
>> not yet have the QAPI generator wired up to support one union as a
>> branch of another larger union?  It leads to extra nesting on the wire
>> [2]
> I don't know the backstory on this limitation. Is it something that
> is very difficult to resolve ? I think it is highly desirable to have
> 'socket': 'SocketAddress' here. It would be a shame to introduce this
> better migration API design and then have it complicated by a possibly
> short term limitation of QAPI.
Agree Daniel. Making different struct just because we have a limitation 
for not able to have union as one of the branch of union, would have 
chances of increasing complexity.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 3:59 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter for initiating a migration stream.
>> This scheme has a significant flaw in it - double encoding of existing
>> URIs to extract migration info.
>>
>> The current patch maps QAPI uri design onto well defined MigrateChannel
>> struct. This modified QAPI helps in preventing multi-level uri
>> encodings ('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>
>> ---
>>   qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 129 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c84fa10e86..79acfcfe4e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1449,12 +1449,108 @@
>>   ##
>>   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>   
>> +
>> +##
>> +# @MigrateSocketAddr:
>> +#
>> +# To support different type of socket.
>> +#
>> +# @socket-type: Different type of socket connections.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateSocketAddr',
>> +  'data': {'socket-type': 'SocketAddress' } }
> I'd really like this struct to go away, but if it must exist,
> then call this field 'addr', as I think 'socket-type' is overly
> verbose.
In v3 patchset, I have already changed from 'socket-type' to 'data'.
>> +
>> +##
>> +# @MigrateExecAddr:
>> + #
>> + # Since 8.0
>> + ##
>> +{ 'struct': 'MigrateExecAddr',
>> +   'data' : {'data': ['str'] } }
> Instead of having the field called 'data' lets me more
> descriptive, and perhaps rename the struct too:
>
>   { 'struct': 'MigrateCommand',
>      'data' : {'args': ['str'] } }
>
> Any thoughts on whether we should allow for setting env varibles
> too ?

Daniel, won't 'MigrateCommand' be too generic ? I am of the opinion 
that, if its related to 'exec' transport, the struct name should reflect 
that ?

I did not get your question allowing setting environment variables. 
Could you explain it in more detail ?

>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'data': 'InetSocketAddress' } }
> InetSocketAddress is a plain struct, so I think we can use
> that directly, no ?
Yes, we can use it directly. Just to keep consistency with other 
transport mechanisms, I made a separate struct even for rdma.
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>> +{ 'union' : 'MigrateAddress',
>> +  'base' : { 'transport' : 'MigrateTransport'},
>> +  'discriminator' : 'transport',
>> +  'data' : {
>> +    'socket' : 'MigrateSocketAddr',
>> +    'exec' : 'MigrateExecAddr',
>> +    'rdma': 'MigrateRdmaAddr' } }
> Ideally this would be
>
>     'data' : {
>       'socket' : 'SocketAddress',
>       'exec' : 'MigrateCommand',
>       'rdma': 'InetSocketAddress' } }
>
> though the first SocketAddress isn't possible unless it is easy to
> lift the QAPI limitation.

Yes, I agree with you Daniel. If we can fix the first one - 
SocketAddress one, can we also allow ['str'] to also be directly 
represented by modifying QAPI ?

ex: 'exec': ['str'] ... something like this ?

>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                        "addr": { "transport": "socket",
>> +#                                  "socket-type": { "type': "inet',
>> +#                                                   "host": "10.12.34.9",
>> +#                                                   "port": "1050" } } } } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "exec",
>> +#                                 "exec": ["/bin/nc", "-U",
>> +#                                          "/some/sock" ] } } } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "rdma",
>> +#                                 "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', '*channel': 'MigrateChannel', '*blk': 'bool',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> IIRC, the intention was to allow multiple channels to be set in a
> follow up to this series. If so that would require adding yet another
> field as an array of MigrateChannel.  Should we just go for the
> array straight away, and just limit it to 1 element  as a runtime
> check ? eg
>
>    'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
>             '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
Yes, I got your point Daniel. But I feel it is better to introduce it in 
follow up series along with introducing different channel types (main, 
data, postcopy). It would then also make sense to introduce a list of 
'MigrateChannel'.
> With regards,
> Daniel
Regards,
Het Gala

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

[Het Gala]

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

'*channels' will be of the form ['MigrateChannel']. Ex: In the list the 
first channel will be of type 'main' and other channel could be of type 
'data' for multifd.

> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

On 09/02/23 4:10 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
>> In existing senario, 'migrate' QAPI argument - string uri, is encoded
>> twice to extract migration parameters for stream connection. This is
>> not a good representation of migration wire protocol as it is a data
>> encoding scheme within a data encoding scheme. Qemu should be able to
>> directly work with results from QAPI without having to do a second
>> level parsing.
>> Modified 'migrate' QAPI design supports well defined MigrateChannel
>> struct which plays important role in avoiding double encoding
>> of uri strings.
>>
>> qemu_uri_parsing() parses uri string (kept for backward
>> compatibility) and populate the MigrateChannel struct parameters.
>> Migration code flow for all required migration transport types -
>> socket, exec and rdma is modified.
>> diff --git a/migration/migration.c b/migration/migration.c
>> index f6dd8dbb03..accbf72a18 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -63,6 +63,7 @@
>>   #include "sysemu/cpus.h"
>>   #include "yank_functions.h"
>>   #include "sysemu/qtest.h"
>> +#include "qemu/sockets.h"
>>   #include "ui/qemu-spice.h"
>>   
>>   #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>> @@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static bool migrate_uri_parse(const char *uri,
>> +                              MigrateChannel **channel,
>> +                              Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>> +
>> +    if (strstart(uri, "exec:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
> Huh, what is the purpose of using 'str_split_at_comma' ? The format
> of this is  "exec:<shell command>", because it is run as:
>
>       const char *argv[] = { "/bin/sh", "-c", command, NULL };
>
> we should not be trying to parse the bit after 'exec:' at all,
> and certainly not splitting it on commas which makes no sense
> for a shell command.
>
> I would have expected something more like this:
>
>      strList **tail = &addrs->u.exec.data;
>      QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>      QAPI_LIST_APPEND(tail, g_strdup("-c"));
>      QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));

Oh, my bad Daniel. I thought for exec as string it would represent 
something like "exec:/bin/sh,-c,<shell-command>". But you are right.

Because I interpreted it wrongly, I wanted to include this function 
'str_split_at_comma' and that's the reason, I had to introduce first 
patch unecessarily.

Thankyou for pointing it out Daniel. I will look into it properly in the 
upcoming patchset, and your solution also makes sense.

>> +    addrs = channel->addr;
>> +    saddr = channel->addr->u.socket.socket_type;
>> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
>> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> +            migrate_protocol_allow_multi_channels(true);
>> +            socket_start_outgoing_migration(s, saddr, &local_err);
>> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> This is probably a sign that  fd_start_outgoing_migration() should
> be folded into the socket_start_outgoing_migration() method, but
> that's a separate cleanup from this patch.
I agree Daniel. 'fd' being a part of SocketAddress, here need to show it 
explicitly makes less sense.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Thu, Feb 09, 2023 at 06:41:41PM +0530, Het Gala wrote:
> 
> On 09/02/23 3:59 pm, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter for initiating a migration stream.
> > > This scheme has a significant flaw in it - double encoding of existing
> > > URIs to extract migration info.
> > > 
> > > The current patch maps QAPI uri design onto well defined MigrateChannel
> > > struct. This modified QAPI helps in preventing multi-level uri
> > > encodings ('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>
> > > ---
> > >   qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 129 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index c84fa10e86..79acfcfe4e 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -1449,12 +1449,108 @@
> > >   ##
> > >   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > > +
> > > +##
> > > +# @MigrateSocketAddr:
> > > +#
> > > +# To support different type of socket.
> > > +#
> > > +# @socket-type: Different type of socket connections.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateSocketAddr',
> > > +  'data': {'socket-type': 'SocketAddress' } }
> > I'd really like this struct to go away, but if it must exist,
> > then call this field 'addr', as I think 'socket-type' is overly
> > verbose.
> In v3 patchset, I have already changed from 'socket-type' to 'data'.



> > > +
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > +   'data' : {'data': ['str'] } }
> > Instead of having the field called 'data' lets me more
> > descriptive, and perhaps rename the struct too:
> > 
> >   { 'struct': 'MigrateCommand',
> >      'data' : {'args': ['str'] } }
> > 
> > Any thoughts on whether we should allow for setting env varibles
> > too ?
> 
> Daniel, won't 'MigrateCommand' be too generic ? I am of the opinion that, if
> its related to 'exec' transport, the struct name should reflect that ?

Mostly I'm indicating that it is not really an address that
we're providing, it is a command argv,  so felt the struct
could reflect that. We could do  MigrateExecCommand.

> I did not get your question allowing setting environment variables. Could
> you explain it in more detail ?

When spawning processes, execvp() lets use provide argv + env. If
env is not provided, we inherit from QEMU. Currently we're only
providing argv, so I was wondering if we should allow env too.
Probably overkill, but at least having the 'MigrateCommand'
struct lets us add it later.

> 
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > +   'data' : {'data': 'InetSocketAddress' } }
> > InetSocketAddress is a plain struct, so I think we can use
> > that directly, no ?
> Yes, we can use it directly. Just to keep consistency with other transport
> mechanisms, I made a separate struct even for rdma.
> > > +
> > > +##
> > > +# @MigrateAddress:
> > > +#
> > > +# The options available for communication transport mechanisms for migration
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'union' : 'MigrateAddress',
> > > +  'base' : { 'transport' : 'MigrateTransport'},
> > > +  'discriminator' : 'transport',
> > > +  'data' : {
> > > +    'socket' : 'MigrateSocketAddr',
> > > +    'exec' : 'MigrateExecAddr',
> > > +    'rdma': 'MigrateRdmaAddr' } }
> > Ideally this would be
> > 
> >     'data' : {
> >       'socket' : 'SocketAddress',
> >       'exec' : 'MigrateCommand',
> >       'rdma': 'InetSocketAddress' } }
> > 
> > though the first SocketAddress isn't possible unless it is easy to
> > lift the QAPI limitation.
> 
> Yes, I agree with you Daniel. If we can fix the first one - SocketAddress
> one, can we also allow ['str'] to also be directly represented by modifying
> QAPI ?
> 
> ex: 'exec': ['str'] ... something like this ?

No, I think it is important to use a struct for 'exec' to allow
future expansion of parameters.


> > > +# -> { "execute": "migrate",
> > > +#      "arguments": {
> > > +#          "channel": { "channeltype": "main",
> > > +#                        "addr": { "transport": "socket",
> > > +#                                  "socket-type": { "type': "inet',
> > > +#                                                   "host": "10.12.34.9",
> > > +#                                                   "port": "1050" } } } } }
> > > +# <- { "return": {} }
> > > +#
> > > +# -> { "execute": "migrate",
> > > +#      "arguments": {
> > > +#          "channel": { "channeltype": "main",
> > > +#                       "addr": { "transport": "exec",
> > > +#                                 "exec": ["/bin/nc", "-U",
> > > +#                                          "/some/sock" ] } } } }
> > > +# <- { "return": {} }
> > > +#
> > > +# -> { "execute": "migrate",
> > > +#      "arguments": {
> > > +#          "channel": { "channeltype": "main",
> > > +#                       "addr": { "transport": "rdma",
> > > +#                                 "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', '*channel': 'MigrateChannel', '*blk': 'bool',
> > > +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> > IIRC, the intention was to allow multiple channels to be set in a
> > follow up to this series. If so that would require adding yet another
> > field as an array of MigrateChannel.  Should we just go for the
> > array straight away, and just limit it to 1 element  as a runtime
> > check ? eg
> > 
> >    'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
> >             '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> Yes, I got your point Daniel. But I feel it is better to introduce it in
> follow up series along with introducing different channel types (main, data,
> postcopy). It would then also make sense to introduce a list of
> 'MigrateChannel'.

Right, that means if we release QEMU 8.0.0 with the 'channel' parameter,
and your next series doesn't get merged until 8.1.0, we're stuck
supporting both 'channel' and 'channels'.

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

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

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Het Gala]

On 09/02/23 5:30 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> renamed hmp_split_at_comma() --> str_split_at_comma()
>> Shifted helper function to qapi-util.c file.
> Not an appropriate home.
>
> If we have to split up a string in QAPI/QMP, we screwed up.  Let me
> explain.
>
> In QAPI/QMP, data is not to be encoded in strings, it is to be
> represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".
>
> When you find yourself parsing QAPI/QMP string values, you're dealing
> with a case where we violated this interface design principle.  Happens,
> but the proper home for code helping to deal with this is *not* qapi/.
> Simply because QAPI is not about parsing strings.

Yes Markus, I agree with you. But we are also supporting 'uri':'str' 
right now for backward compatibility. So splitting of string might have 
needed. But I misunderstood a crucial part of exec string, and hence had 
to introduce this patch in the first place. Daniel made my understanding 
clear now in the 4th patch.

Now, there is no need to introduce this patch. In the upcoming patchset 
version, this patch will be dropped.

>>                                               Give external linkage, as
>> this function will be handy in coming commit for migration.
> It already has external linkage.
>
>> Minor correction:
>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
> This is not actually a correction :)
>
> Omitting the second operand of a conditional expression like x ?: y is
> equivalent to x ? x : y, except it evaluates x only once.
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedocs_gcc_Conditionals.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=gAsWrkiPm3MCpqkWQzGYo9-M2_2bkxfDAGmW8lgXmOAnW2YoDs5AhxGPt-Sc5xI3&s=Onmed5Fm0PImk6PD44bAvu8yQDrGuYU44yRYAw3Abjs&e=
Ack. Thankyou for getting it into my knowledge. Will take care from next 
time
> Besides, please don't mix code motion with code changes.
Yes sure Markus. I apologise for not knowing it earlier and introducing 
such a patch, but lesson learned :)
> [...]
Regards,
Het Gala

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Het Gala]

On 09/02/23 5:32 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:55AM +0000, Het Gala wrote:
>> renamed hmp_split_at_comma() --> str_split_at_comma()
>> Shifted helper function to qapi-util.c file. Give external linkage, as
>> this function will be handy in coming commit for migration.
>>
>> Minor correction:
>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
>>
>> 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>
>> ---
>>   include/monitor/hmp.h  |  1 -
>>   include/qapi/util.h    |  1 +
>>   monitor/hmp-cmds.c     | 19 -------------------
>>   net/net-hmp-cmds.c     |  2 +-
>>   qapi/qapi-util.c       | 19 +++++++++++++++++++
>>   stats/stats-hmp-cmds.c |  2 +-
>>   6 files changed, 22 insertions(+), 22 deletions(-)
> I expect this patch can be dropped, since I don't believe it is
> correct to be using it in patch 2. I left comments in that other
> patch with more details.
Yes Daniel. This patch will be dropped in upcoming version of this 
patchset.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Het Gala]

On 09/02/23 5:35 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
>> hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
>> into well defined MigrateChannel struct with help of
>> migrate_channel_from_qdict().
>> hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
>> argument (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 | 105 ++++++++++++++++++++++++++++++++-
>>   migration/migration.c          |  15 ++++-
>>   2 files changed, 116 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index ef25bc8929..a9f65ded7a 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -32,6 +32,101 @@
>>   #include "sysemu/runstate.h"
>>   #include "ui/qemu-spice.h"
>>   
>> +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");
>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>> +    MigrateChannelType channel_type;
>> +    MigrateTransport transport;
>> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new(SocketAddress, 1);
>> +    SocketAddressType type;
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>> +
>> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
>> +                                   channeltype, -1, &err);
>> +    if (channel_type < 0) {
>> +        goto end;
>> +    }
>> +
>> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
>> +                                transport_str, -1, &err);
>> +    if (transport < 0) {
>> +        goto end;
>> +    }
>> +
>> +    type = qapi_enum_parse(&SocketAddressType_lookup,
>> +                           socketaddr_type, -1, &err);
>> +    if (type < 0) {
>> +        goto end;
>> +    }
>> +
>> +    addr->transport = transport;
>> +
>> +    switch (transport) {
>> +    case MIGRATE_TRANSPORT_SOCKET:
>> +        saddr->type = type;
>> +
>> +        switch (type) {
>> +        case SOCKET_ADDRESS_TYPE_INET:
>> +            saddr->u.inet.host = (char *)inet_host;
>> +            saddr->u.inet.port = (char *)inet_port;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_UNIX:
>> +            saddr->u.q_unix.path = (char *)unix_path;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_VSOCK:
>> +            saddr->u.vsock.cid = (char *)vsock_cid;
>> +            saddr->u.vsock.port = (char *)vsock_port;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_FD:
>> +            saddr->u.fd.str = (char *)fd;
>> +            break;
>> +        default:
>> +            error_setg(errp, "%s: Unknown socket type %d",
>> +                       __func__, saddr->type);
>> +            break;
>> +        }
>> +
>> +        addr->u.socket.socket_type = saddr;
>> +        break;
>> +    case MIGRATE_TRANSPORT_EXEC:
>> +        addr->u.exec.data = (strList *)exec;
>> +         break;
>> +    case MIGRATE_TRANSPORT_RDMA:
>> +        isock->host = (char *)inet_host;
>> +        isock->port = (char *)inet_port;
>> +        addr->u.rdma.data = isock;
>> +        break;
>> +    default:
>> +        error_setg(errp, "%s: Unknown migrate transport type %d",
>> +                   __func__, addr->transport);
>> +        break;
>> +    }
>> +
>> +    val->channeltype = channel_type;
>> +    val->addr = addr;
>> +    *channel = val;
>> +    return true;
>> +
>> +end:
>> +    error_propagate(errp, err);
>> +    return false;
>> +}
>> +
>> +
>>   void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>>   {
>>       MigrationInfo *info;
>> @@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>       const char *uri = qdict_get_str(qdict, "uri");
>>       Error *err = NULL;
>>   
>> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
>> +    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(uri, channel, !!blk, blk, !!inc, inc,
>>                   false, false, true, resume, &err);
>> +    qapi_free_MigrateChannel(channel);
>>       if (hmp_handle_error(mon, err)) {
>>           return;
>>       }
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 7a14aa98d8..f6dd8dbb03 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>       return true;
>>   }
>>   
>> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
>> -                 bool has_inc, bool inc, bool has_detach, bool detach,
>> -                 bool has_resume, bool resume, Error **errp)
>> +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>> +                 bool blk, bool has_inc, bool inc, bool has_detach,
>> +                 bool detach, bool has_resume, bool resume, Error **errp)
>>   {
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           }
>>       }
>>   
>> +    /*
>> +     * Having preliminary checks for uri and channel
>> +     */
>> +    if (uri && channel) {
>> +        error_setg(errp, "uri and channels options should be"
> s/should be/are/, also best to quote parameter names, eg
>
>      error_setg(errp,
>                 "'uri' and 'channels' options are mutually exclusive");
Ack.
>> +                          "mutually exclusive");
>> +        return;
>> +    }
>> +
> This change for qmp_migrate will need to be in patch 2.
>
> QEMU needs to compile & pass tests successfully after each individual
> patch. Currently it'll fail on patch 2, because the code generator
> wil emit the new qmp_migrate API declaration signature, but the change
> to the implementation signature is here in patch 3.

Yes Daniel, it will fail on patch 2. My understanding was that, even if 
sometimes individual patches dont compile properly, the final series of 
all the patches should be compiled properly. But I understand your point.

I have a small concern here Daniel, if you could help me resolve it?
- There is a similar issue in patch 4. Where some function parameters 
are to be changed. But that function responds to both source and 
destination side changes. So though patch 4 contains all the source side 
changes, it does not take into account destination side changes and it 
does not compile entirely. And after destination side changes are inside 
patch 6, the dependencies are resolved. How is it possible to compile 
individual patches in this case, because then each patch should also 
have some significant meaning to all the changes. So, in that way, 
source side changes in one commit and destination side changes in 
another commit makes more sense right ?
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
> > On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter for initiating a migration stream.
> > > This scheme has a significant flaw in it - double encoding of existing
> > > URIs to extract migration info.
> > > 
> > > The current patch maps QAPI uri design onto well defined MigrateChannel
> > > struct. This modified QAPI helps in preventing multi-level uri
> > > encodings ('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>
> > > ---
> > >  qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 129 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index c84fa10e86..79acfcfe4e 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -1449,12 +1449,108 @@
> > >  ##
> > >  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > >  
> > > +##
> > > +# @MigrateTransport:
> > > +#
> > > +# The supported communication transport mechanisms for migration
> > > +#
> > > +# @socket: Supported communication type between two devices for migration.
> > > +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > > +#          'fd' already
> > > +#
> > > +# @exec: Supported communication type to redirect migration stream into file.
> > > +#
> > > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'enum': 'MigrateTransport',
> > > +  'data': ['socket', 'exec', 'rdma'] }
> > > +
> > > +##
> > > +# @MigrateSocketAddr:
> > > +#
> > > +# To support different type of socket.
> > > +#
> > > +# @socket-type: Different type of socket connections.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateSocketAddr',
> > > +  'data': {'socket-type': 'SocketAddress' } }
> > 
> > Here, you use 'socket-type',...
> > 
> > > +
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > +   'data' : {'data': ['str'] } }
> > 
> > Inconsistent on whether you have a space before :.  Most of our qapi
> > files prefer the layout:
> > 
> > 'key': 'value'
> > 
> > that is, no space before, one space after.  It doesn't affect
> > correctness, but a consistent visual style is worth striving for.
> > 
> > > +
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > +   'data' : {'data': 'InetSocketAddress' } }
> > 
> > ...while these branches supply everything else under 'data'. Also,
> > while you documented @socket-type above, you did not document @data in
> > either of these two types.  [1]
> > 
> > > +
> > > +##
> > > +# @MigrateAddress:
> > > +#
> > > +# The options available for communication transport mechanisms for migration
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'union' : 'MigrateAddress',
> > > +  'base' : { 'transport' : 'MigrateTransport'},
> > > +  'discriminator' : 'transport',
> > > +  'data' : {
> > > +    'socket' : 'MigrateSocketAddr',
> > > +    'exec' : 'MigrateExecAddr',
> > > +    'rdma': 'MigrateRdmaAddr' } }
> > 
> > Another example of inconsistent spacing around :.
> > 
> > I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
> > is that SocketAddress is itself a discriminated union, and Markus does
> > not yet have the QAPI generator wired up to support one union as a
> > branch of another larger union?  It leads to extra nesting on the wire
> > [2]
> 
> I don't know the backstory on this limitation. Is it something that
> is very difficult to resolve ? I think it is highly desirable to have
> 'socket': 'SocketAddress' here. It would be a shame to introduce this
> better migration API design and then have it complicated by a possibly
> short term limitation of QAPI.

So to understand this better I did this change on top of Het's
patch:

diff --git a/qapi/migration.json b/qapi/migration.json
index 79acfcfe4e..bbc3e66ad6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1467,18 +1467,6 @@
 { 'enum': 'MigrateTransport',
   'data': ['socket', 'exec', 'rdma'] }
 
-##
-# @MigrateSocketAddr:
-#
-# To support different type of socket.
-#
-# @socket-type: Different type of socket connections.
-#
-# Since 8.0
-##
-{ 'struct': 'MigrateSocketAddr',
-  'data': {'socket-type': 'SocketAddress' } }
-
 ##
 # @MigrateExecAddr:
  #
@@ -1487,14 +1475,6 @@
 { 'struct': 'MigrateExecAddr',
    'data' : {'data': ['str'] } }
 
-##
-# @MigrateRdmaAddr:
-#
-# Since 8.0
-##
-{ 'struct': 'MigrateRdmaAddr',
-   'data' : {'data': 'InetSocketAddress' } }
-
 ##
 # @MigrateAddress:
 #
@@ -1506,9 +1486,9 @@
   'base' : { 'transport' : 'MigrateTransport'},
   'discriminator' : 'transport',
   'data' : {
-    'socket' : 'MigrateSocketAddr',
+    'socket' : 'SocketAddress',
     'exec' : 'MigrateExecAddr',
-    'rdma': 'MigrateRdmaAddr' } }
+    'rdma': 'InetSocketAddress' } }
 
 ##
 # @MigrateChannelType:


That results in a build error

  /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
  ../qapi/migration.json: In union 'MigrateAddress':
  ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'

To understand what the limitation of QAPI generation is, I blindly
disabled this check

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cd8661125c..cb5c0385bd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -653,7 +653,7 @@ def check(self, schema, seen):
                         "branch '%s' is not a value of %s"
                         % (v.name, self.tag_member.type.describe()))
                 if (not isinstance(v.type, QAPISchemaObjectType)
-                        or v.type.variants):
+                        or v.type.variants) and False:
                     raise QAPISemError(
                         self.info,
                         "%s cannot use %s"
@@ -664,7 +664,8 @@ def check_clash(self, info, seen):
         for v in self.variants:
             # Reset seen map for each variant, since qapi names from one
             # branch do not affect another branch
-            v.type.check_clash(info, dict(seen))
+            #v.type.check_clash(info, dict(seen))
+            pass
 
 
 class QAPISchemaMember:


After doing that, the QAPI code generated handled the union-inside-union
case without any problems that I can see. The generated code looks sane
and it compiles correctly.

So is this actually just a case of overly strict input validation  in
the QAPI schema parser ?  If so, what's the correct way to adapt the
checks to permit this usage.

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

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 16191 bytes --]


On 09/02/23 5:39 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
>> In existing senario, 'migrate' QAPI argument - string uri, is encoded
>> twice to extract migration parameters for stream connection. This is
>> not a good representation of migration wire protocol as it is a data
>> encoding scheme within a data encoding scheme. Qemu should be able to
>> directly work with results from QAPI without having to do a second
>> level parsing.
>> Modified 'migrate' QAPI design supports well defined MigrateChannel
>> struct which plays important role in avoiding double encoding
>> of uri strings.
>>
>> qemu_uri_parsing() parses uri string (kept for backward
>> compatibility) and populate the MigrateChannel struct parameters.
>> Migration code flow for all required migration transport types -
>> socket, exec and rdma is modified.
>>
>> 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/exec.c      | 31 ++++++++++++++++--
>>   migration/exec.h      |  4 ++-
>>   migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
>>   migration/rdma.c      | 30 +++++------------
>>   migration/rdma.h      |  3 +-
>>   migration/socket.c    | 21 ++++--------
>>   migration/socket.h    |  3 +-
>>   7 files changed, 110 insertions(+), 57 deletions(-)
>>
>> diff --git a/migration/exec.c b/migration/exec.c
>> index 375d2e1b54..4fa9819792 100644
>> --- a/migration/exec.c
>> +++ b/migration/exec.c
>> @@ -23,14 +23,39 @@
>>   #include "migration.h"
>>   #include "io/channel-command.h"
>>   #include "trace.h"
>> +#include "qapi/error.h"
>>   
>>   
>> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>> +void init_exec_array(strList *command, const char *argv[], Error **errp)
>> +{
>> +    int i = 0;
>> +    strList *lst;
>> +
>> +    for (lst = command; lst ; lst = lst->next) {
>> +        argv[i++] = lst->value;
>> +    }
>> +
>> +    /*
>> +     * Considering exec command always has 3 arguments to execute
>> +     * a command directly from the bash itself.
>> +     */
>> +    if (i > 3) {
>> +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
>> +        return;
>> +    }
> By the time this check fires, the for() loop above has already
> done out of bounds writes on argv[].
Ack. check should be before for loop.
>> +
>> +    argv[i] = NULL;
>> +    return;
>> +}
>> +
>> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
>> +                                   Error **errp)
>>   {
>>       QIOChannel *ioc;
>> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>> +    const char *argv[4];
>> +    init_exec_array(command, argv, errp);
> If someone invokes 'migrate' with the old URI style, the
> strList will be 3 elements, and thus argv[4] is safe.
>
> If someone invokes 'migrate' with thue new MigrateChannel style,
> the strList can be arbitrarily long and thus argv[4] will be
> risk of overflow.

Okay, Can you give me an example where strList can be very long in the 
new MigrateChannel ? because in that case,

trace_migration_exec_outgoing(argv[2]);

will also be not correct right. Will have to come up with something that 
is dynamic ?

>>   
>> -    trace_migration_exec_outgoing(command);
>> +    trace_migration_exec_outgoing(argv[2]);
>>       ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>>                                                       O_RDWR,
>>                                                       errp));
>> diff --git a/migration/exec.h b/migration/exec.h
>> index b210ffde7a..5b39ba6cbb 100644
>> --- a/migration/exec.h
>> +++ b/migration/exec.h
>> @@ -19,8 +19,10 @@
>>   
>>   #ifndef QEMU_MIGRATION_EXEC_H
>>   #define QEMU_MIGRATION_EXEC_H
>> +void init_exec_array(strList *command, const char *argv[], Error **errp);
>> +
>>   void exec_start_incoming_migration(const char *host_port, Error **errp);
>>   
>> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
>>                                      Error **errp);
>>   #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index f6dd8dbb03..accbf72a18 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -63,6 +63,7 @@
>>   #include "sysemu/cpus.h"
>>   #include "yank_functions.h"
>>   #include "sysemu/qtest.h"
>> +#include "qemu/sockets.h"
>>   #include "ui/qemu-spice.h"
>>   
>>   #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>> @@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static bool migrate_uri_parse(const char *uri,
>> +                              MigrateChannel **channel,
>> +                              Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>> +
>> +    if (strstart(uri, "exec:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
>> +    } else if (strstart(uri, "rdma:", NULL) &&
>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
>> +        addrs->u.rdma.data = isock;
>> +    } else if (strstart(uri, "tcp:", NULL) ||
>> +                strstart(uri, "unix:", NULL) ||
>> +                strstart(uri, "vsock:", NULL) ||
>> +                strstart(uri, "fd:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
>> +        saddr = socket_parse(uri, &local_err);
>> +        addrs->u.socket.socket_type = saddr;
>> +    }
>> +
>> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
>> +    val->addr = addrs;
>> +    *channel = val;
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>>       const char *p = NULL;
>> @@ -2469,7 +2508,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>   {
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> -    const char *p = NULL;
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>>   
>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>>                            has_resume && resume, errp)) {
>> @@ -2490,22 +2530,29 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>           error_setg(errp, "uri and channels options should be"
>>                             "mutually exclusive");
>>           return;
>> +    } else if (uri && !migrate_uri_parse(uri, &channel, &local_err)) {
>> +        error_setg(errp, "Error parsing uri");
>> +        return;
>>       }
>>   
>>       migrate_protocol_allow_multi_channels(false);
>> -    if (strstart(uri, "tcp:", &p) ||
>> -        strstart(uri, "unix:", NULL) ||
>> -        strstart(uri, "vsock:", NULL)) {
>> -        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)) {
>> -        rdma_start_outgoing_migration(s, p, &local_err);
>> -#endif
>> -    } else if (strstart(uri, "exec:", &p)) {
>> -        exec_start_outgoing_migration(s, p, &local_err);
>> -    } else if (strstart(uri, "fd:", &p)) {
>> -        fd_start_outgoing_migration(s, p, &local_err);
>> +    addrs = channel->addr;
>> +    saddr = channel->addr->u.socket.socket_type;
>> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
>> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> +            migrate_protocol_allow_multi_channels(true);
>> +            socket_start_outgoing_migration(s, saddr, &local_err);
>> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>> +        }
>> +    #ifdef CONFIG_RDMA
>> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
>> +        rdma_start_outgoing_migration(s, addrs->u.rdma.data, &local_err);
>> +    #endif
>> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
>> +        exec_start_outgoing_migration(s, addrs->u.exec.data, &local_err);
>>       } else {
>>           if (!(has_resume && resume)) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 288eadc2d2..48f49add6f 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -316,7 +316,6 @@ typedef struct RDMALocalBlocks {
>>   typedef struct RDMAContext {
>>       char *host;
>>       int port;
>> -    char *host_port;
>>   
>>       RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
>>   
>> @@ -2449,9 +2448,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>>           rdma->channel = NULL;
>>       }
>>       g_free(rdma->host);
>> -    g_free(rdma->host_port);
>>       rdma->host = NULL;
>> -    rdma->host_port = NULL;
>>   }
>>   
>>   
>> @@ -2733,28 +2730,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
>>       rdma_return_path->is_return_path = true;
>>   }
>>   
>> -static void *qemu_rdma_data_init(const char *host_port, Error **errp)
>> +static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
>>   {
>>       RDMAContext *rdma = NULL;
>> -    InetSocketAddress *addr;
>>   
>> -    if (host_port) {
>> +    if (saddr) {
>>           rdma = g_new0(RDMAContext, 1);
>>           rdma->current_index = -1;
>>           rdma->current_chunk = -1;
>>   
>> -        addr = g_new(InetSocketAddress, 1);
>> -        if (!inet_parse(addr, host_port, NULL)) {
>> -            rdma->port = atoi(addr->port);
>> -            rdma->host = g_strdup(addr->host);
>> -            rdma->host_port = g_strdup(host_port);
>> -        } else {
>> -            ERROR(errp, "bad RDMA migration address '%s'", host_port);
>> -            g_free(rdma);
>> -            rdma = NULL;
>> -        }
>> -
>> -        qapi_free_InetSocketAddress(addr);
>> +        rdma->host = g_strdup(saddr->host);
>> +        rdma->port = atoi(saddr->port);
>>       }
>>   
>>       return rdma;
>> @@ -3354,6 +3340,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>                                               .private_data_len = sizeof(cap),
>>                                            };
>>       RDMAContext *rdma_return_path = NULL;
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>>       struct rdma_cm_event *cm_event;
>>       struct ibv_context *verbs;
>>       int ret = -EINVAL;
>> @@ -4152,14 +4139,13 @@ err:
>>       error_propagate(errp, local_err);
>>       if (rdma) {
>>           g_free(rdma->host);
>> -        g_free(rdma->host_port);
>>       }
>>       g_free(rdma);
>>       g_free(rdma_return_path);
>>   }
>>   
>>   void rdma_start_outgoing_migration(void *opaque,
>> -                            const char *host_port, Error **errp)
>> +                            InetSocketAddress *addr, Error **errp)
>>   {
>>       MigrationState *s = opaque;
>>       RDMAContext *rdma_return_path = NULL;
>> @@ -4172,7 +4158,7 @@ void rdma_start_outgoing_migration(void *opaque,
>>           return;
>>       }
>>   
>> -    rdma = qemu_rdma_data_init(host_port, errp);
>> +    rdma = qemu_rdma_data_init(addr, errp);
>>       if (rdma == NULL) {
>>           goto err;
>>       }
>> @@ -4193,7 +4179,7 @@ void rdma_start_outgoing_migration(void *opaque,
>>   
>>       /* RDMA postcopy need a separate queue pair for return path */
>>       if (migrate_postcopy()) {
>> -        rdma_return_path = qemu_rdma_data_init(host_port, errp);
>> +        rdma_return_path = qemu_rdma_data_init(addr, errp);
>>   
>>           if (rdma_return_path == NULL) {
>>               goto return_path_err;
>> diff --git a/migration/rdma.h b/migration/rdma.h
>> index de2ba09dc5..8d9978e1a9 100644
>> --- a/migration/rdma.h
>> +++ b/migration/rdma.h
>> @@ -13,11 +13,12 @@
>>    * later.  See the COPYING file in the top-level directory.
>>    *
>>    */
>> +#include "io/channel-socket.h"
>>   
>>   #ifndef QEMU_MIGRATION_RDMA_H
>>   #define QEMU_MIGRATION_RDMA_H
>>   
>> -void rdma_start_outgoing_migration(void *opaque, const char *host_port,
>> +void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *addr,
>>                                      Error **errp);
>>   
>>   void rdma_start_incoming_migration(const char *host_port, Error **errp);
>> diff --git a/migration/socket.c b/migration/socket.c
>> index e6fdf3c5e1..c751e0bfc1 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -27,6 +27,8 @@
>>   #include "io/net-listener.h"
>>   #include "trace.h"
>>   #include "postcopy-ram.h"
>> +#include "qapi/clone-visitor.h"
>> +#include "qapi/qapi-visit-sockets.h"
>>   
>>   struct SocketOutgoingArgs {
>>       SocketAddress *saddr;
>> @@ -107,19 +109,20 @@ out:
>>       object_unref(OBJECT(sioc));
>>   }
>>   
>> -static void
>> -socket_start_outgoing_migration_internal(MigrationState *s,
>> +void socket_start_outgoing_migration(MigrationState *s,
>>                                            SocketAddress *saddr,
>>                                            Error **errp)
>>   {
>>       QIOChannelSocket *sioc = qio_channel_socket_new();
>>       struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>> +    SocketAddress *addr = g_new0(SocketAddress, 1);
>> +    addr = QAPI_CLONE(SocketAddress, saddr);
>>   
>>       data->s = s;
>>   
>>       /* in case previous migration leaked it */
>>       qapi_free_SocketAddress(outgoing_args.saddr);
>> -    outgoing_args.saddr = saddr;
>> +    outgoing_args.saddr = addr;
>>   
>>       if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
>>           data->hostname = g_strdup(saddr->u.inet.host);
>> @@ -134,18 +137,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
>>                                        NULL);
>>   }
>>   
>> -void socket_start_outgoing_migration(MigrationState *s,
>> -                                     const char *str,
>> -                                     Error **errp)
>> -{
>> -    Error *err = NULL;
>> -    SocketAddress *saddr = socket_parse(str, &err);
>> -    if (!err) {
>> -        socket_start_outgoing_migration_internal(s, saddr, &err);
>> -    }
>> -    error_propagate(errp, err);
>> -}
>> -
>>   static void socket_accept_incoming_migration(QIONetListener *listener,
>>                                                QIOChannelSocket *cioc,
>>                                                gpointer opaque)
>> diff --git a/migration/socket.h b/migration/socket.h
>> index dc54df4e6c..95c9c166ec 100644
>> --- a/migration/socket.h
>> +++ b/migration/socket.h
>> @@ -19,6 +19,7 @@
>>   
>>   #include "io/channel.h"
>>   #include "io/task.h"
>> +#include "io/channel-socket.h"
>>   
>>   void socket_send_channel_create(QIOTaskFunc f, void *data);
>>   QIOChannel *socket_send_channel_create_sync(Error **errp);
>> @@ -26,6 +27,6 @@ int socket_send_channel_destroy(QIOChannel *send);
>>   
>>   void socket_start_incoming_migration(const char *str, Error **errp);
>>   
>> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
>> +void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr,
>>                                        Error **errp);
>>   #endif
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 16835 bytes --]

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Het Gala]

On 09/02/23 5:42 pm, Juan Quintela wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> renamed hmp_split_at_comma() --> str_split_at_comma()
>>> Shifted helper function to qapi-util.c file.
>> Not an appropriate home.
> I don't have an opinion here.
>
>> If we have to split up a string in QAPI/QMP, we screwed up.  Let me
>> explain.
>>
>> In QAPI/QMP, data is not to be encoded in strings, it is to be
>> represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".
> In this case, we are already screwed up O:-)
>
> the uri value in migration has to be split.
> What this series does is creating a new way to express the command
> (something on the lines that you describe), but we still have to
> maintain the old way of doing it for some time, so we need this
> function.
>
>> When you find yourself parsing QAPI/QMP string values, you're dealing
>> with a case where we violated this interface design principle.  Happens,
>> but the proper home for code helping to deal with this is *not* qapi/.
>> Simply because QAPI is not about parsing strings.
> Ok, I drop my review-by.
>
> See why I was asking for reviews from QAPI libvirt folks for this code O:-)
>
>>>                                               Give external linkage, as
>>> this function will be handy in coming commit for migration.
>> It already has external linkage.
>>
>>> Minor correction:
>>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
>> This is not actually a correction :)
>>
>> Omitting the second operand of a conditional expression like x ?: y is
>> equivalent to x ? x : y, except it evaluates x only once.
> You are (way) more C layer than me.
>
> Once told that, I think that what he wanted to do is making this c
> standard, no gcc standard.
>
> Once told that, I think that every C compiler worth its salt has this
> extension.
>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedocs_gcc_Conditionals.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=b-y7IKTlkPluPqpf6lI-BKMLDSrV5JJRRzU39eSq6CpslAITuH5Cxi6l_XugJfkM&s=Z1FND19C0lNnL8v7t_pifjUxyCxbHC8OL2fX-euPRb4&e=
>>
>> Besides, please don't mix code motion with code changes.
> Agreed.
Thankyou for your comments Juan. After discussing on the same with 
Daniel, this patch will be dropped in the next patchset version as the 
str_split func. it would not be necessary in the first place.
> Later, Juan.
Regards,
Het Gala

Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Daniel P. Berrangé]

On Thu, Feb 09, 2023 at 07:08:13PM +0530, Het Gala wrote:
> 
> On 09/02/23 5:35 pm, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
> > > hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
> > > into well defined MigrateChannel struct with help of
> > > migrate_channel_from_qdict().
> > > hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
> > > argument (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 | 105 ++++++++++++++++++++++++++++++++-
> > >   migration/migration.c          |  15 ++++-
> > >   2 files changed, 116 insertions(+), 4 deletions(-)
> > > 


> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 7a14aa98d8..f6dd8dbb03 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
> > >       return true;
> > >   }
> > > -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > > -                 bool has_inc, bool inc, bool has_detach, bool detach,
> > > -                 bool has_resume, bool resume, Error **errp)
> > > +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
> > > +                 bool blk, bool has_inc, bool inc, bool has_detach,
> > > +                 bool detach, bool has_resume, bool resume, Error **errp)
> > >   {
> > >       Error *local_err = NULL;
> > >       MigrationState *s = migrate_get_current();
> > > @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > >           }
> > >       }
> > > +    /*
> > > +     * Having preliminary checks for uri and channel
> > > +     */
> > > +    if (uri && channel) {
> > > +        error_setg(errp, "uri and channels options should be"
> > s/should be/are/, also best to quote parameter names, eg
> > 
> >      error_setg(errp,
> >                 "'uri' and 'channels' options are mutually exclusive");
> Ack.
> > > +                          "mutually exclusive");
> > > +        return;
> > > +    }
> > > +
> > This change for qmp_migrate will need to be in patch 2.
> > 
> > QEMU needs to compile & pass tests successfully after each individual
> > patch. Currently it'll fail on patch 2, because the code generator
> > wil emit the new qmp_migrate API declaration signature, but the change
> > to the implementation signature is here in patch 3.
> 
> Yes Daniel, it will fail on patch 2. My understanding was that, even if
> sometimes individual patches dont compile properly, the final series of all
> the patches should be compiled properly. But I understand your point.

No, unfortunately we require the strict behaviour, where *every* individual
commit must compile and pass unit tests.

The reason for this is that when chasing regression bugs, it is common for
developers to use 'git bisect' to test compilation across a range of
releases. 'git bisect' will land on completely arbitrary commits, so it
is critical that every QEMU commit in the repo must compile and pass
tests. It isn't sufficient for just the end of the series to compile.

> I have a small concern here Daniel, if you could help me resolve it?
> - There is a similar issue in patch 4. Where some function parameters are to
> be changed. But that function responds to both source and destination side
> changes. So though patch 4 contains all the source side changes, it does not
> take into account destination side changes and it does not compile entirely.
> And after destination side changes are inside patch 6, the dependencies are
> resolved. How is it possible to compile individual patches in this case,
> because then each patch should also have some significant meaning to all the
> changes. So, in that way, source side changes in one commit and destination
> side changes in another commit makes more sense right ?

If there is code that is shared between src + dst, that may put constraints
on how you split up the patches.

Possibly a split like this may avoid having dependancy problems:

  * Patch intoduces the 'MigrateAddress' struct and related child
    objects, but *not* the MigrateChannel struct.
    
  * Patch introduces code that can parse a 'uri' and spit out a
    'MigrateAddress' struct.
    
  * Patch converts internal socket backend to accept MigrateAddress,
    with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
    
  * Patch converts internal exec backend to accept MigrateAddress
    with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
    
  * Patch converts internal rdma backend to accept MigrateAddress
    with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
    
  * Patch converts 'migrate' command to accept MigrateChannel param
    directly
  
  * Patch converts 'migrate_incoming' command to accept MigrateChannel
    param directly.

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

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Daniel P. Berrangé]

On Thu, Feb 09, 2023 at 07:24:48PM +0530, Het Gala wrote:
> 
> On 09/02/23 5:39 pm, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
> > > In existing senario, 'migrate' QAPI argument - string uri, is encoded
> > > twice to extract migration parameters for stream connection. This is
> > > not a good representation of migration wire protocol as it is a data
> > > encoding scheme within a data encoding scheme. Qemu should be able to
> > > directly work with results from QAPI without having to do a second
> > > level parsing.
> > > Modified 'migrate' QAPI design supports well defined MigrateChannel
> > > struct which plays important role in avoiding double encoding
> > > of uri strings.
> > > 
> > > qemu_uri_parsing() parses uri string (kept for backward
> > > compatibility) and populate the MigrateChannel struct parameters.
> > > Migration code flow for all required migration transport types -
> > > socket, exec and rdma is modified.
> > > 
> > > 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/exec.c      | 31 ++++++++++++++++--
> > >   migration/exec.h      |  4 ++-
> > >   migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
> > >   migration/rdma.c      | 30 +++++------------
> > >   migration/rdma.h      |  3 +-
> > >   migration/socket.c    | 21 ++++--------
> > >   migration/socket.h    |  3 +-
> > >   7 files changed, 110 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/migration/exec.c b/migration/exec.c
> > > index 375d2e1b54..4fa9819792 100644
> > > --- a/migration/exec.c
> > > +++ b/migration/exec.c
> > > @@ -23,14 +23,39 @@
> > >   #include "migration.h"
> > >   #include "io/channel-command.h"
> > >   #include "trace.h"
> > > +#include "qapi/error.h"
> > > -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> > > +void init_exec_array(strList *command, const char *argv[], Error **errp)
> > > +{
> > > +    int i = 0;
> > > +    strList *lst;
> > > +
> > > +    for (lst = command; lst ; lst = lst->next) {
> > > +        argv[i++] = lst->value;
> > > +    }
> > > +
> > > +    /*
> > > +     * Considering exec command always has 3 arguments to execute
> > > +     * a command directly from the bash itself.
> > > +     */
> > > +    if (i > 3) {
> > > +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
> > > +        return;
> > > +    }
> > By the time this check fires, the for() loop above has already
> > done out of bounds writes on argv[].
> Ack. check should be before for loop.
> > > +
> > > +    argv[i] = NULL;
> > > +    return;
> > > +}
> > > +
> > > +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> > > +                                   Error **errp)
> > >   {
> > >       QIOChannel *ioc;
> > > -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> > > +    const char *argv[4];
> > > +    init_exec_array(command, argv, errp);
> > If someone invokes 'migrate' with the old URI style, the
> > strList will be 3 elements, and thus argv[4] is safe.
> > 
> > If someone invokes 'migrate' with thue new MigrateChannel style,
> > the strList can be arbitrarily long and thus argv[4] will be
> > risk of overflow.
> 
> Okay, Can you give me an example where strList can be very long in the new
> MigrateChannel ? because in that case,

The new MigrateAddress struct allows the user to have arbitrary
command args, so for example I would expect to be able to do


 { "execute": "migrate",
     "arguments": {
         "channel": { "channeltype": "main",
                      "addr": { "transport": "exec",
                                "exec": ["/bin/ssh",
				         "-p", "6000",
					 "-l", "root",
					 "-o", "CheckHostIP=no",
					 "-o", "ConnectTimeout=15",
                                         "somehost" ] } } } }



> 
> trace_migration_exec_outgoing(argv[2]);
> 
> will also be not correct right. Will have to come up with something that is
> dynamic ?

Yes, that will need addressing too.

We already need to convert from strList to char ** in order
to call qio_channel_command_new_spawn.

Given that, you can use g_strjoinv(" ", argv) to generate a
combined string that can be given to the trace func.


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

Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> renamed hmp_split_at_comma() --> str_split_at_comma()
>>> Shifted helper function to qapi-util.c file.
>>
>> Not an appropriate home.
>
> I don't have an opinion here.
>
>> If we have to split up a string in QAPI/QMP, we screwed up.  Let me
>> explain.
>>
>> In QAPI/QMP, data is not to be encoded in strings, it is to be
>> represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".
>
> In this case, we are already screwed up O:-)

A loooong time ago :)

> the uri value in migration has to be split.
> What this series does is creating a new way to express the command
> (something on the lines that you describe), but we still have to
> maintain the old way of doing it for some time, so we need this
> function.

And that's fine.  I just want it to stay out of qapi/, to avoid giving
people the idea that splitting string is something QAPI wants you to do.

>> When you find yourself parsing QAPI/QMP string values, you're dealing
>> with a case where we violated this interface design principle.  Happens,
>> but the proper home for code helping to deal with this is *not* qapi/.
>> Simply because QAPI is not about parsing strings.
>
> Ok, I drop my review-by.
>
> See why I was asking for reviews from QAPI libvirt folks for this code O:-)

Better late than never (I hope).

>>>                                              Give external linkage, as
>>> this function will be handy in coming commit for migration.
>>
>> It already has external linkage.
>>
>>> Minor correction:
>>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
>>
>> This is not actually a correction :)
>>
>> Omitting the second operand of a conditional expression like x ?: y is
>> equivalent to x ? x : y, except it evaluates x only once.
>
> You are (way) more C layer than me.

Guilty as charged.

> Once told that, I think that what he wanted to do is making this c
> standard, no gcc standard.
>
> Once told that, I think that every C compiler worth its salt has this
> extension.

There are hundreds of uses in the tree.

>> https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html
>>
>> Besides, please don't mix code motion with code changes.
>
> Agreed.
>
> Later, Juan.

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:

[...]

>> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
>> is that SocketAddress is itself a discriminated union, and Markus does
>> not yet have the QAPI generator wired up to support one union as a
>> branch of another larger union?  It leads to extra nesting on the wire
>> [2]
>
> I don't know the backstory on this limitation. Is it something that
> is very difficult to resolve ? I think it is highly desirable to have
> 'socket': 'SocketAddress' here. It would be a shame to introduce this
> better migration API design and then have it complicated by a possibly
> short term limitation of QAPI.

We evolve the QAPI language to satisfy concrete use cases.  If you could
use a language improvement, make a case for it, and we'll see what we
can do within a time frame that works for you.  Better than ugly
work-arounds on the silent assumption the language cannot be adapted.

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 9:56 pm, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
> [...]
>
>>> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
>>> is that SocketAddress is itself a discriminated union, and Markus does
>>> not yet have the QAPI generator wired up to support one union as a
>>> branch of another larger union?  It leads to extra nesting on the wire
>>> [2]
>> I don't know the backstory on this limitation. Is it something that
>> is very difficult to resolve ? I think it is highly desirable to have
>> 'socket': 'SocketAddress' here. It would be a shame to introduce this
>> better migration API design and then have it complicated by a possibly
>> short term limitation of QAPI.
> We evolve the QAPI language to satisfy concrete use cases.  If you could
> use a language improvement, make a case for it, and we'll see what we
> can do within a time frame that works for you.  Better than ugly
> work-arounds on the silent assumption the language cannot be adapted.

Hi Markus, Daniel has already left some comments on version 2, patch 2. 
In short, we want to make a union, where some of the branches of that 
union can call directly to another known union like 'SocketAddress'. 
Right now, QAPI does not allow to directly call a union inside a branch 
of union, and need to make a spearate struct, and then call a union 
again, and increases complexity of QAPI design which is contrary to what 
we aim for.

We can further discuss on the response Markus has given in the latest v2 
patch 2, and aim to, how we can evolve QAPI languge and reduce complexity :)


Regards,
Het Gala

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 7:08 pm, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote:
>> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
>>> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>>>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>>>> of destination interface and corresponding port number in the form
>>>> of a unified string 'uri' parameter for initiating a migration stream.
>>>> This scheme has a significant flaw in it - double encoding of existing
>>>> URIs to extract migration info.
>>>>
>>>> The current patch maps QAPI uri design onto well defined MigrateChannel
>>>> struct. This modified QAPI helps in preventing multi-level uri
>>>> encodings ('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>
>>>> ---
>>>>   qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 129 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index c84fa10e86..79acfcfe4e 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1449,12 +1449,108 @@
>>>>   ##
>>>>   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>>   
>>>> +##
>>>> +# @MigrateTransport:
>>>> +#
>>>> +# The supported communication transport mechanisms for migration
>>>> +#
>>>> +# @socket: Supported communication type between two devices for migration.
>>>> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>>>> +#          'fd' already
>>>> +#
>>>> +# @exec: Supported communication type to redirect migration stream into file.
>>>> +#
>>>> +# @rdma: Supported communication type to redirect rdma type migration stream.
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'enum': 'MigrateTransport',
>>>> +  'data': ['socket', 'exec', 'rdma'] }
>>>> +
>>>> +##
>>>> +# @MigrateSocketAddr:
>>>> +#
>>>> +# To support different type of socket.
>>>> +#
>>>> +# @socket-type: Different type of socket connections.
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'struct': 'MigrateSocketAddr',
>>>> +  'data': {'socket-type': 'SocketAddress' } }
>>> Here, you use 'socket-type',...
>>>
>>>> +
>>>> +##
>>>> +# @MigrateExecAddr:
>>>> + #
>>>> + # Since 8.0
>>>> + ##
>>>> +{ 'struct': 'MigrateExecAddr',
>>>> +   'data' : {'data': ['str'] } }
>>> Inconsistent on whether you have a space before :.  Most of our qapi
>>> files prefer the layout:
>>>
>>> 'key': 'value'
>>>
>>> that is, no space before, one space after.  It doesn't affect
>>> correctness, but a consistent visual style is worth striving for.
>>>
>>>> +
>>>> +##
>>>> +# @MigrateRdmaAddr:
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'struct': 'MigrateRdmaAddr',
>>>> +   'data' : {'data': 'InetSocketAddress' } }
>>> ...while these branches supply everything else under 'data'. Also,
>>> while you documented @socket-type above, you did not document @data in
>>> either of these two types.  [1]
>>>
>>>> +
>>>> +##
>>>> +# @MigrateAddress:
>>>> +#
>>>> +# The options available for communication transport mechanisms for migration
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'union' : 'MigrateAddress',
>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>> +  'discriminator' : 'transport',
>>>> +  'data' : {
>>>> +    'socket' : 'MigrateSocketAddr',
>>>> +    'exec' : 'MigrateExecAddr',
>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>> Another example of inconsistent spacing around :.
>>>
>>> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
>>> is that SocketAddress is itself a discriminated union, and Markus does
>>> not yet have the QAPI generator wired up to support one union as a
>>> branch of another larger union?  It leads to extra nesting on the wire
>>> [2]
>> I don't know the backstory on this limitation. Is it something that
>> is very difficult to resolve ? I think it is highly desirable to have
>> 'socket': 'SocketAddress' here. It would be a shame to introduce this
>> better migration API design and then have it complicated by a possibly
>> short term limitation of QAPI.
> So to understand this better I did this change on top of Het's
> patch:
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 79acfcfe4e..bbc3e66ad6 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1467,18 +1467,6 @@
>   { 'enum': 'MigrateTransport',
>     'data': ['socket', 'exec', 'rdma'] }
>   
> -##
> -# @MigrateSocketAddr:
> -#
> -# To support different type of socket.
> -#
> -# @socket-type: Different type of socket connections.
> -#
> -# Since 8.0
> -##
> -{ 'struct': 'MigrateSocketAddr',
> -  'data': {'socket-type': 'SocketAddress' } }
> -
>   ##
>   # @MigrateExecAddr:
>    #
> @@ -1487,14 +1475,6 @@
>   { 'struct': 'MigrateExecAddr',
>      'data' : {'data': ['str'] } }
>   
> -##
> -# @MigrateRdmaAddr:
> -#
> -# Since 8.0
> -##
> -{ 'struct': 'MigrateRdmaAddr',
> -   'data' : {'data': 'InetSocketAddress' } }
> -
>   ##
>   # @MigrateAddress:
>   #
> @@ -1506,9 +1486,9 @@
>     'base' : { 'transport' : 'MigrateTransport'},
>     'discriminator' : 'transport',
>     'data' : {
> -    'socket' : 'MigrateSocketAddr',
> +    'socket' : 'SocketAddress',
>       'exec' : 'MigrateExecAddr',
> -    'rdma': 'MigrateRdmaAddr' } }
> +    'rdma': 'InetSocketAddress' } }
>   
>   ##
>   # @MigrateChannelType:
>
>
> That results in a build error
>
>    /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>    ../qapi/migration.json: In union 'MigrateAddress':
>    ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>
> To understand what the limitation of QAPI generation is, I blindly
> disabled this check
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cd8661125c..cb5c0385bd 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -653,7 +653,7 @@ def check(self, schema, seen):
>                           "branch '%s' is not a value of %s"
>                           % (v.name, self.tag_member.type.describe()))
>                   if (not isinstance(v.type, QAPISchemaObjectType)
> -                        or v.type.variants):
> +                        or v.type.variants) and False:
>                       raise QAPISemError(
>                           self.info,
>                           "%s cannot use %s"
> @@ -664,7 +664,8 @@ def check_clash(self, info, seen):
>           for v in self.variants:
>               # Reset seen map for each variant, since qapi names from one
>               # branch do not affect another branch
> -            v.type.check_clash(info, dict(seen))
> +            #v.type.check_clash(info, dict(seen))
> +            pass
>   
>   
>   class QAPISchemaMember:
>
>
> After doing that, the QAPI code generated handled the union-inside-union
> case without any problems that I can see. The generated code looks sane
> and it compiles correctly.
>
> So is this actually just a case of overly strict input validation  in
> the QAPI schema parser ?  If so, what's the correct way to adapt the
> checks to permit this usage.
Could we take advice of Markus and other QAPI maintainers here and 
improve QAPI language here.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Het Gala]

On 09/02/23 7:30 pm, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 07:08:13PM +0530, Het Gala wrote:
>> On 09/02/23 5:35 pm, Daniel P. Berrangé wrote:
>>> On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
>>>> hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
>>>> into well defined MigrateChannel struct with help of
>>>> migrate_channel_from_qdict().
>>>> hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
>>>> argument (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 | 105 ++++++++++++++++++++++++++++++++-
>>>>    migration/migration.c          |  15 ++++-
>>>>    2 files changed, 116 insertions(+), 4 deletions(-)
>>>>
>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 7a14aa98d8..f6dd8dbb03 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>>>        return true;
>>>>    }
>>>> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>> -                 bool has_inc, bool inc, bool has_detach, bool detach,
>>>> -                 bool has_resume, bool resume, Error **errp)
>>>> +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>>> +                 bool blk, bool has_inc, bool inc, bool has_detach,
>>>> +                 bool detach, bool has_resume, bool resume, Error **errp)
>>>>    {
>>>>        Error *local_err = NULL;
>>>>        MigrationState *s = migrate_get_current();
>>>> @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>            }
>>>>        }
>>>> +    /*
>>>> +     * Having preliminary checks for uri and channel
>>>> +     */
>>>> +    if (uri && channel) {
>>>> +        error_setg(errp, "uri and channels options should be"
>>> s/should be/are/, also best to quote parameter names, eg
>>>
>>>       error_setg(errp,
>>>                  "'uri' and 'channels' options are mutually exclusive");
>> Ack.
>>>> +                          "mutually exclusive");
>>>> +        return;
>>>> +    }
>>>> +
>>> This change for qmp_migrate will need to be in patch 2.
>>>
>>> QEMU needs to compile & pass tests successfully after each individual
>>> patch. Currently it'll fail on patch 2, because the code generator
>>> wil emit the new qmp_migrate API declaration signature, but the change
>>> to the implementation signature is here in patch 3.
>> Yes Daniel, it will fail on patch 2. My understanding was that, even if
>> sometimes individual patches dont compile properly, the final series of all
>> the patches should be compiled properly. But I understand your point.
> No, unfortunately we require the strict behaviour, where *every* individual
> commit must compile and pass unit tests.
>
> The reason for this is that when chasing regression bugs, it is common for
> developers to use 'git bisect' to test compilation across a range of
> releases. 'git bisect' will land on completely arbitrary commits, so it
> is critical that every QEMU commit in the repo must compile and pass
> tests. It isn't sufficient for just the end of the series to compile.
>
>> I have a small concern here Daniel, if you could help me resolve it?
>> - There is a similar issue in patch 4. Where some function parameters are to
>> be changed. But that function responds to both source and destination side
>> changes. So though patch 4 contains all the source side changes, it does not
>> take into account destination side changes and it does not compile entirely.
>> And after destination side changes are inside patch 6, the dependencies are
>> resolved. How is it possible to compile individual patches in this case,
>> because then each patch should also have some significant meaning to all the
>> changes. So, in that way, source side changes in one commit and destination
>> side changes in another commit makes more sense right ?
> If there is code that is shared between src + dst, that may put constraints
> on how you split up the patches.
>
> Possibly a split like this may avoid having dependancy problems:
>
>    * Patch intoduces the 'MigrateAddress' struct and related child
>      objects, but *not* the MigrateChannel struct.
>      
>    * Patch introduces code that can parse a 'uri' and spit out a
>      'MigrateAddress' struct.
>      
>    * Patch converts internal socket backend to accept MigrateAddress,
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts internal exec backend to accept MigrateAddress
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts internal rdma backend to accept MigrateAddress
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts 'migrate' command to accept MigrateChannel param
>      directly
>    
>    * Patch converts 'migrate_incoming' command to accept MigrateChannel
>      param directly.
Thankyou Daniel, seems to be a great idea. I will try to restructure the 
patches in a manner that every patch will compile and pass unit tests 
individually.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

On 09/02/23 7:36 pm, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 07:24:48PM +0530, Het Gala wrote:
>> On 09/02/23 5:39 pm, Daniel P. Berrangé wrote:
>>> On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
>>>> In existing senario, 'migrate' QAPI argument - string uri, is encoded
>>>> twice to extract migration parameters for stream connection. This is
>>>> not a good representation of migration wire protocol as it is a data
>>>> encoding scheme within a data encoding scheme. Qemu should be able to
>>>> directly work with results from QAPI without having to do a second
>>>> level parsing.
>>>> Modified 'migrate' QAPI design supports well defined MigrateChannel
>>>> struct which plays important role in avoiding double encoding
>>>> of uri strings.
>>>>
>>>> qemu_uri_parsing() parses uri string (kept for backward
>>>> compatibility) and populate the MigrateChannel struct parameters.
>>>> Migration code flow for all required migration transport types -
>>>> socket, exec and rdma is modified.
>>>>
>>>> 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/exec.c      | 31 ++++++++++++++++--
>>>>    migration/exec.h      |  4 ++-
>>>>    migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
>>>>    migration/rdma.c      | 30 +++++------------
>>>>    migration/rdma.h      |  3 +-
>>>>    migration/socket.c    | 21 ++++--------
>>>>    migration/socket.h    |  3 +-
>>>>    7 files changed, 110 insertions(+), 57 deletions(-)
>>>>
>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>> index 375d2e1b54..4fa9819792 100644
>>>> --- a/migration/exec.c
>>>> +++ b/migration/exec.c
>>>> @@ -23,14 +23,39 @@
>>>>    #include "migration.h"
>>>>    #include "io/channel-command.h"
>>>>    #include "trace.h"
>>>> +#include "qapi/error.h"
>>>> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>>>> +void init_exec_array(strList *command, const char *argv[], Error **errp)
>>>> +{
>>>> +    int i = 0;
>>>> +    strList *lst;
>>>> +
>>>> +    for (lst = command; lst ; lst = lst->next) {
>>>> +        argv[i++] = lst->value;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Considering exec command always has 3 arguments to execute
>>>> +     * a command directly from the bash itself.
>>>> +     */
>>>> +    if (i > 3) {
>>>> +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
>>>> +        return;
>>>> +    }
>>> By the time this check fires, the for() loop above has already
>>> done out of bounds writes on argv[].
>> Ack. check should be before for loop.
>>>> +
>>>> +    argv[i] = NULL;
>>>> +    return;
>>>> +}
>>>> +
>>>> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
>>>> +                                   Error **errp)
>>>>    {
>>>>        QIOChannel *ioc;
>>>> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>>>> +    const char *argv[4];
>>>> +    init_exec_array(command, argv, errp);
>>> If someone invokes 'migrate' with the old URI style, the
>>> strList will be 3 elements, and thus argv[4] is safe.
>>>
>>> If someone invokes 'migrate' with thue new MigrateChannel style,
>>> the strList can be arbitrarily long and thus argv[4] will be
>>> risk of overflow.
>> Okay, Can you give me an example where strList can be very long in the new
>> MigrateChannel ? because in that case,
> The new MigrateAddress struct allows the user to have arbitrary
> command args, so for example I would expect to be able to do
>
>
>   { "execute": "migrate",
>       "arguments": {
>           "channel": { "channeltype": "main",
>                        "addr": { "transport": "exec",
>                                  "exec": ["/bin/ssh",
> 				         "-p", "6000",
> 					 "-l", "root",
> 					 "-o", "CheckHostIP=no",
> 					 "-o", "ConnectTimeout=15",
>                                           "somehost" ] } } } }
>
>
>
>> trace_migration_exec_outgoing(argv[2]);
>>
>> will also be not correct right. Will have to come up with something that is
>> dynamic ?
> Yes, that will need addressing too.
>
> We already need to convert from strList to char ** in order
> to call qio_channel_command_new_spawn.
>
> Given that, you can use g_strjoinv(" ", argv) to generate a
> combined string that can be given to the trace func.
Thankyou Daniel for the example. I understood properly now. Will try to 
handle both the cases - coming from uri as well as coming from 
MigrateChannel struct.
> With regards,
> Daniel
Regards,
Het Gala

QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command)

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

[...]

>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>> +{ 'union' : 'MigrateAddress',
>> +  'base' : { 'transport' : 'MigrateTransport'},
>> +  'discriminator' : 'transport',
>> +  'data' : {
>> +    'socket' : 'MigrateSocketAddr',
>> +    'exec' : 'MigrateExecAddr',
>> +    'rdma': 'MigrateRdmaAddr' } }
>
> Ideally this would be
>
>    'data' : {
>      'socket' : 'SocketAddress',
>      'exec' : 'MigrateCommand',
>      'rdma': 'InetSocketAddress' } }
>
> though the first SocketAddress isn't possible unless it is easy to
> lift the QAPI limitation.

Context: SocketAddress is a QAPI union, and "the QAPI limitation" is

    scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
    ../qapi/migration.json: In union 'MigrateAddress':
    ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'

Emitted by schema.py like this:

                if (not isinstance(v.type, QAPISchemaObjectType)
                        or v.type.variants):
                    raise QAPISemError(
                        self.info,
                        "%s cannot use %s"
                        % (v.describe(self.info), v.type.describe()))

This enforces docs/devel/qapi-code-gen.rst's clause

    The BRANCH's value defines the branch's properties, in particular its
    type.  The type must a struct type.  [...]

Next paragraph:

    In the Client JSON Protocol, a union is represented by an object with
    the common members (from the base type) and the selected branch's
    members.  The two sets of member names must be disjoint.

So, we're splicing in the members of the branch's JSON object.  For that
to even make sense, the branch type needs to map to a JSON object.  This
is fundamental.  It's the first part of the condition in the code
snippet above.

We have two kinds of QAPI types that map to a JSON object: struct and
union.  The second part of the condition restricts to struct.  Unless
I'm missing something (imperfect memory...), this is *not* fundamental,
just a matter of implementing it.  But I'd have to try to be sure.


Instead of simply allowing unions in addition to structs here, I'd like
to go one step further, and fuse the two into "objects".  Let me
explain.

If we abstract from syntax, structs have become almost a special kind of
union.  Unions have a set of common members and sets of variant members,
and a special common member (the tag) selects the set of variant
members.  Structs are unions with zero variants and no tag.

The generator code actually represents both structs and unions as a
common QAPISchemaObjectType already.  QAPI/QMP introspection does the
same: it uses a single meta type 'object' for both.


There is another spot where only structs are allowed: a struct or
union's base type.  That restriction will be awkward to lift, as I made
the mistake of baking the assumption "object type has at most one tag
member" into QAPI/QMP introspection .

[...]

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/02/23 6:52 pm, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 06:41:41PM +0530, Het Gala wrote:
>> On 09/02/23 3:59 pm, Daniel P. Berrangé wrote:
>>> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>>>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>>>> of destination interface and corresponding port number in the form
>>>> of a unified string 'uri' parameter for initiating a migration stream.
>>>> This scheme has a significant flaw in it - double encoding of existing
>>>> URIs to extract migration info.
>>>>
>>>> The current patch maps QAPI uri design onto well defined MigrateChannel
>>>> struct. This modified QAPI helps in preventing multi-level uri
>>>> encodings ('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>
>>>> ---
>>>>    qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 129 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index c84fa10e86..79acfcfe4e 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1449,12 +1449,108 @@
>>>>    ##
>>>>    { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>> +
>>>> +##
>>>> +# @MigrateSocketAddr:
>>>> +#
>>>> +# To support different type of socket.
>>>> +#
>>>> +# @socket-type: Different type of socket connections.
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'struct': 'MigrateSocketAddr',
>>>> +  'data': {'socket-type': 'SocketAddress' } }
>>> I'd really like this struct to go away, but if it must exist,
>>> then call this field 'addr', as I think 'socket-type' is overly
>>> verbose.
>> In v3 patchset, I have already changed from 'socket-type' to 'data'.
>
>
>>>> +
>>>> +##
>>>> +# @MigrateExecAddr:
>>>> + #
>>>> + # Since 8.0
>>>> + ##
>>>> +{ 'struct': 'MigrateExecAddr',
>>>> +   'data' : {'data': ['str'] } }
>>> Instead of having the field called 'data' lets me more
>>> descriptive, and perhaps rename the struct too:
>>>
>>>    { 'struct': 'MigrateCommand',
>>>       'data' : {'args': ['str'] } }
>>>
>>> Any thoughts on whether we should allow for setting env varibles
>>> too ?
>> Daniel, won't 'MigrateCommand' be too generic ? I am of the opinion that, if
>> its related to 'exec' transport, the struct name should reflect that ?
> Mostly I'm indicating that it is not really an address that
> we're providing, it is a command argv,  so felt the struct
> could reflect that. We could do  MigrateExecCommand.
Yes. 'MigrateExecCommand' seems more appropriate.
>> I did not get your question allowing setting environment variables. Could
>> you explain it in more detail ?
> When spawning processes, execvp() lets use provide argv + env. If
> env is not provided, we inherit from QEMU. Currently we're only
> providing argv, so I was wondering if we should allow env too.
> Probably overkill, but at least having the 'MigrateCommand'
> struct lets us add it later.
Okay, now I get your point. Thanks for the explanation Daniel.
>>>> +##
>>>> +# @MigrateRdmaAddr:
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'struct': 'MigrateRdmaAddr',
>>>> +   'data' : {'data': 'InetSocketAddress' } }
>>> InetSocketAddress is a plain struct, so I think we can use
>>> that directly, no ?
>> Yes, we can use it directly. Just to keep consistency with other transport
>> mechanisms, I made a separate struct even for rdma.
>>>> +
>>>> +##
>>>> +# @MigrateAddress:
>>>> +#
>>>> +# The options available for communication transport mechanisms for migration
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'union' : 'MigrateAddress',
>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>> +  'discriminator' : 'transport',
>>>> +  'data' : {
>>>> +    'socket' : 'MigrateSocketAddr',
>>>> +    'exec' : 'MigrateExecAddr',
>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>> Ideally this would be
>>>
>>>      'data' : {
>>>        'socket' : 'SocketAddress',
>>>        'exec' : 'MigrateCommand',
>>>        'rdma': 'InetSocketAddress' } }
>>>
>>> though the first SocketAddress isn't possible unless it is easy to
>>> lift the QAPI limitation.
>> Yes, I agree with you Daniel. If we can fix the first one - SocketAddress
>> one, can we also allow ['str'] to also be directly represented by modifying
>> QAPI ?
>>
>> ex: 'exec': ['str'] ... something like this ?
> No, I think it is important to use a struct for 'exec' to allow
> future expansion of parameters.
Yes, got your point of exec paramter expansion idea from env variable 
concept.
>>>> +# -> { "execute": "migrate",
>>>> +#      "arguments": {
>>>> +#          "channel": { "channeltype": "main",
>>>> +#                        "addr": { "transport": "socket",
>>>> +#                                  "socket-type": { "type': "inet',
>>>> +#                                                   "host": "10.12.34.9",
>>>> +#                                                   "port": "1050" } } } } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +# -> { "execute": "migrate",
>>>> +#      "arguments": {
>>>> +#          "channel": { "channeltype": "main",
>>>> +#                       "addr": { "transport": "exec",
>>>> +#                                 "exec": ["/bin/nc", "-U",
>>>> +#                                          "/some/sock" ] } } } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +# -> { "execute": "migrate",
>>>> +#      "arguments": {
>>>> +#          "channel": { "channeltype": "main",
>>>> +#                       "addr": { "transport": "rdma",
>>>> +#                                 "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', '*channel': 'MigrateChannel', '*blk': 'bool',
>>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>> IIRC, the intention was to allow multiple channels to be set in a
>>> follow up to this series. If so that would require adding yet another
>>> field as an array of MigrateChannel.  Should we just go for the
>>> array straight away, and just limit it to 1 element  as a runtime
>>> check ? eg
>>>
>>>     'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
>>>              '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>> Yes, I got your point Daniel. But I feel it is better to introduce it in
>> follow up series along with introducing different channel types (main, data,
>> postcopy). It would then also make sense to introduce a list of
>> 'MigrateChannel'.
> Right, that means if we release QEMU 8.0.0 with the 'channel' parameter,
> and your next series doesn't get merged until 8.1.0, we're stuck
> supporting both 'channel' and 'channels'.
Okay, understood. It might become messy. If we implement 'channels' 
right from start, we would just need to remove the check in the later 
series but still supporting 'channels' and unecessary does not need to 
include anything intermediate.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote:

[...]

>> I don't know the backstory on this limitation. Is it something that
>> is very difficult to resolve ? I think it is highly desirable to have
>> 'socket': 'SocketAddress' here. It would be a shame to introduce this
>> better migration API design and then have it complicated by a possibly
>> short term limitation of QAPI.
>
> So to understand this better I did this change on top of Het's
> patch:
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 79acfcfe4e..bbc3e66ad6 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1467,18 +1467,6 @@
>  { 'enum': 'MigrateTransport',
>    'data': ['socket', 'exec', 'rdma'] }
>  
> -##
> -# @MigrateSocketAddr:
> -#
> -# To support different type of socket.
> -#
> -# @socket-type: Different type of socket connections.
> -#
> -# Since 8.0
> -##
> -{ 'struct': 'MigrateSocketAddr',
> -  'data': {'socket-type': 'SocketAddress' } }
> -
>  ##
>  # @MigrateExecAddr:
>   #
> @@ -1487,14 +1475,6 @@
>  { 'struct': 'MigrateExecAddr',
>     'data' : {'data': ['str'] } }
>  
> -##
> -# @MigrateRdmaAddr:
> -#
> -# Since 8.0
> -##
> -{ 'struct': 'MigrateRdmaAddr',
> -   'data' : {'data': 'InetSocketAddress' } }
> -
>  ##
>  # @MigrateAddress:
>  #
> @@ -1506,9 +1486,9 @@
>    'base' : { 'transport' : 'MigrateTransport'},
>    'discriminator' : 'transport',
>    'data' : {
> -    'socket' : 'MigrateSocketAddr',
> +    'socket' : 'SocketAddress',
>      'exec' : 'MigrateExecAddr',
> -    'rdma': 'MigrateRdmaAddr' } }
> +    'rdma': 'InetSocketAddress' } }
>  
>  ##
>  # @MigrateChannelType:
>
>
> That results in a build error
>
>   /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>   ../qapi/migration.json: In union 'MigrateAddress':
>   ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>
> To understand what the limitation of QAPI generation is, I blindly
> disabled this check
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cd8661125c..cb5c0385bd 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -653,7 +653,7 @@ def check(self, schema, seen):
>                          "branch '%s' is not a value of %s"
>                          % (v.name, self.tag_member.type.describe()))
>                  if (not isinstance(v.type, QAPISchemaObjectType)
> -                        or v.type.variants):
> +                        or v.type.variants) and False:
>                      raise QAPISemError(
>                          self.info,
>                          "%s cannot use %s"
> @@ -664,7 +664,8 @@ def check_clash(self, info, seen):
>          for v in self.variants:
>              # Reset seen map for each variant, since qapi names from one
>              # branch do not affect another branch
> -            v.type.check_clash(info, dict(seen))
> +            #v.type.check_clash(info, dict(seen))
> +            pass
>  
>  
>  class QAPISchemaMember:
>
>
> After doing that, the QAPI code generated handled the union-inside-union
> case without any problems that I can see. The generated code looks sane
> and it compiles correctly.

As you discovered, the code was designed to treat structs and unions the
same.  But there are holes.

> So is this actually just a case of overly strict input validation  in
> the QAPI schema parser ?  If so, what's the correct way to adapt the
> checks to permit this usage.

The check you disabled guards holes.  There's at least this one in
QAPISchemaObjectType.check_clash():

    # Check that the members of this type do not cause duplicate JSON members,
    # and update seen to track the members seen so far. Report any errors
    # on behalf of info, which is not necessarily self.info
    def check_clash(self, info, seen):
        assert self._checked
        assert not self.variants       # not implemented
        for m in self.members:
            m.check_clash(info, seen)

Re: QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command)

[Het Gala]

On 10/02/23 12:54 pm, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> [...]
>
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for migration
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'union' : 'MigrateAddress',
>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>> +  'discriminator' : 'transport',
>>> +  'data' : {
>>> +    'socket' : 'MigrateSocketAddr',
>>> +    'exec' : 'MigrateExecAddr',
>>> +    'rdma': 'MigrateRdmaAddr' } }
>> Ideally this would be
>>
>>     'data' : {
>>       'socket' : 'SocketAddress',
>>       'exec' : 'MigrateCommand',
>>       'rdma': 'InetSocketAddress' } }
>>
>> though the first SocketAddress isn't possible unless it is easy to
>> lift the QAPI limitation.
> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
>
>      scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>      ../qapi/migration.json: In union 'MigrateAddress':
>      ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>
> Emitted by schema.py like this:
>
>                  if (not isinstance(v.type, QAPISchemaObjectType)
>                          or v.type.variants):
>                      raise QAPISemError(
>                          self.info,
>                          "%s cannot use %s"
>                          % (v.describe(self.info), v.type.describe()))
>
> This enforces docs/devel/qapi-code-gen.rst's clause
>
>      The BRANCH's value defines the branch's properties, in particular its
>      type.  The type must a struct type.  [...]
>
> Next paragraph:
>
>      In the Client JSON Protocol, a union is represented by an object with
>      the common members (from the base type) and the selected branch's
>      members.  The two sets of member names must be disjoint.
>
> So, we're splicing in the members of the branch's JSON object.  For that
> to even make sense, the branch type needs to map to a JSON object.  This
> is fundamental.  It's the first part of the condition in the code
> snippet above.
>
> We have two kinds of QAPI types that map to a JSON object: struct and
> union.  The second part of the condition restricts to struct.  Unless
> I'm missing something (imperfect memory...), this is *not* fundamental,
> just a matter of implementing it.  But I'd have to try to be sure.
>
>
> Instead of simply allowing unions in addition to structs here, I'd like
> to go one step further, and fuse the two into "objects".  Let me
> explain.
>
> If we abstract from syntax, structs have become almost a special kind of
> union.  Unions have a set of common members and sets of variant members,
> and a special common member (the tag) selects the set of variant
> members.  Structs are unions with zero variants and no tag.
>
> The generator code actually represents both structs and unions as a
> common QAPISchemaObjectType already.  QAPI/QMP introspection does the
> same: it uses a single meta type 'object' for both.
>
>
> There is another spot where only structs are allowed: a struct or
> union's base type.  That restriction will be awkward to lift, as I made
> the mistake of baking the assumption "object type has at most one tag
> member" into QAPI/QMP introspection .

Hi Markus, thankyou for explaning in such detail. I tried to understand 
of what you explained.

So IIUC, you mentioned the QAPI generator treats both structs and unions 
same, but basically in the schema.py checks is where it tries to 
distinguish between the two ? and because of the fact that 
docs/devel/qapi-code-gen.rst states that for a union, it's branches must 
be 'struct', and that's the reason it gives an error ?

If that's the case, can we improve on our checks and allow union as a 
part of branch of a union ? or something else ?

or I may have completely misunderstood most of the part 😅. Please let 
me know

>
> [...]
Regards,
Het Gala

Re: QAPI unions as branches / unifying struct and union types

[Markus Armbruster]

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

> On 10/02/23 12:54 pm, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> [...]
>>
>>>> +##
>>>> +# @MigrateAddress:
>>>> +#
>>>> +# The options available for communication transport mechanisms for migration
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'union' : 'MigrateAddress',
>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>> +  'discriminator' : 'transport',
>>>> +  'data' : {
>>>> +    'socket' : 'MigrateSocketAddr',
>>>> +    'exec' : 'MigrateExecAddr',
>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>>
>>> Ideally this would be
>>>
>>>     'data' : {
>>>       'socket' : 'SocketAddress',
>>>       'exec' : 'MigrateCommand',
>>>       'rdma': 'InetSocketAddress' } }
>>>
>>> though the first SocketAddress isn't possible unless it is easy to
>>> lift the QAPI limitation.
>>
>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
>>
>>      scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>>      ../qapi/migration.json: In union 'MigrateAddress':
>>      ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>>
>> Emitted by schema.py like this:
>>
>>                  if (not isinstance(v.type, QAPISchemaObjectType)
>>                          or v.type.variants):
>>                      raise QAPISemError(
>>                          self.info,
>>                          "%s cannot use %s"
>>                          % (v.describe(self.info), v.type.describe()))
>>
>> This enforces docs/devel/qapi-code-gen.rst's clause
>>
>>      The BRANCH's value defines the branch's properties, in particular its
>>      type.  The type must a struct type.  [...]
>>
>> Next paragraph:
>>
>>      In the Client JSON Protocol, a union is represented by an object with
>>      the common members (from the base type) and the selected branch's
>>      members.  The two sets of member names must be disjoint.
>>
>> So, we're splicing in the members of the branch's JSON object.  For that
>> to even make sense, the branch type needs to map to a JSON object.  This
>> is fundamental.  It's the first part of the condition in the code
>> snippet above.
>>
>> We have two kinds of QAPI types that map to a JSON object: struct and
>> union.  The second part of the condition restricts to struct.  Unless
>> I'm missing something (imperfect memory...), this is *not* fundamental,
>> just a matter of implementing it.  But I'd have to try to be sure.
>>
>>
>> Instead of simply allowing unions in addition to structs here, I'd like
>> to go one step further, and fuse the two into "objects".  Let me
>> explain.
>>
>> If we abstract from syntax, structs have become almost a special kind of
>> union.  Unions have a set of common members and sets of variant members,
>> and a special common member (the tag) selects the set of variant
>> members.  Structs are unions with zero variants and no tag.
>>
>> The generator code actually represents both structs and unions as a
>> common QAPISchemaObjectType already.  QAPI/QMP introspection does the
>> same: it uses a single meta type 'object' for both.
>>
>>
>> There is another spot where only structs are allowed: a struct or
>> union's base type.  That restriction will be awkward to lift, as I made
>> the mistake of baking the assumption "object type has at most one tag
>> member" into QAPI/QMP introspection .
>
> Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained.
>
> So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ?

Permit me a brief digression into history.

The initial QAPI design language provided product types (structs) and
sum types (unions containing exactly one of several types, and a tag
member that tells which one).  The two are orthogonal.

These unions turned out rather awkward.

The unions we have today are more general.  They have common members,
and one of them is the tag member, of enumeration type.  For each tag
value, they have variant members.  Both the common members and each tag
value's variant members are given as struct types.

What if the tag's enumeration type is empty, i.e. has no values?  We get
a union with no variant members, only common ones.  Isn't that a struct?

Not quite.  To get a struct, we also have to drop the tag member.  It
has no possible values anyway.

You see, struct types are almost a special case of today's union types.
To overcome "almost", we can introduce the notion of "object type":

* An object type has common members, one of them can be a tag member, of
  enumeration type, not empty.  For each tag value, it additionally has
  variant members.

* A union type is an object type with a tag member and variant members.

* A struct type is an object type without tag member and variant
  members.

The QAPI generator code already made the jump to this object type
notion.  It transform the special cases into the general case at first
opportunity, in QAPISchema._def_struct_type() and ._def_union_type().

*Except* we haven't implemented support for variant members in a few
places where they cannot occur now, e.g. as a tag value's variant.  This
is the restriction you ran into.

I'd like to make the jump to object type in the QAPI schema language,
too.  But that's not a prerequisite to lifting the restriction.

> If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ?

I believe we can implement the missing parts and relax the checks.  But
to be sure, we need to try.

> or I may have completely misunderstood most of the part 😅. Please let me know

More questions?

Re: QAPI unions as branches / unifying struct and union types

[Het Gala]

On 14/02/23 3:46 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 10/02/23 12:54 pm, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>> [...]
>>>
>>>>> +##
>>>>> +# @MigrateAddress:
>>>>> +#
>>>>> +# The options available for communication transport mechanisms for migration
>>>>> +#
>>>>> +# Since 8.0
>>>>> +##
>>>>> +{ 'union' : 'MigrateAddress',
>>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>>> +  'discriminator' : 'transport',
>>>>> +  'data' : {
>>>>> +    'socket' : 'MigrateSocketAddr',
>>>>> +    'exec' : 'MigrateExecAddr',
>>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>>> Ideally this would be
>>>>
>>>>      'data' : {
>>>>        'socket' : 'SocketAddress',
>>>>        'exec' : 'MigrateCommand',
>>>>        'rdma': 'InetSocketAddress' } }
>>>>
>>>> though the first SocketAddress isn't possible unless it is easy to
>>>> lift the QAPI limitation.
>>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
>>>
>>>       scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>>>       ../qapi/migration.json: In union 'MigrateAddress':
>>>       ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>>>
>>> Emitted by schema.py like this:
>>>
>>>                   if (not isinstance(v.type, QAPISchemaObjectType)
>>>                           or v.type.variants):
>>>                       raise QAPISemError(
>>>                           self.info,
>>>                           "%s cannot use %s"
>>>                           % (v.describe(self.info), v.type.describe()))
>>>
>>> This enforces docs/devel/qapi-code-gen.rst's clause
>>>
>>>       The BRANCH's value defines the branch's properties, in particular its
>>>       type.  The type must a struct type.  [...]
>>>
>>> Next paragraph:
>>>
>>>       In the Client JSON Protocol, a union is represented by an object with
>>>       the common members (from the base type) and the selected branch's
>>>       members.  The two sets of member names must be disjoint.
>>>
>>> So, we're splicing in the members of the branch's JSON object.  For that
>>> to even make sense, the branch type needs to map to a JSON object.  This
>>> is fundamental.  It's the first part of the condition in the code
>>> snippet above.
>>>
>>> We have two kinds of QAPI types that map to a JSON object: struct and
>>> union.  The second part of the condition restricts to struct.  Unless
>>> I'm missing something (imperfect memory...), this is *not* fundamental,
>>> just a matter of implementing it.  But I'd have to try to be sure.
>>>
>>>
>>> Instead of simply allowing unions in addition to structs here, I'd like
>>> to go one step further, and fuse the two into "objects".  Let me
>>> explain.
>>>
>>> If we abstract from syntax, structs have become almost a special kind of
>>> union.  Unions have a set of common members and sets of variant members,
>>> and a special common member (the tag) selects the set of variant
>>> members.  Structs are unions with zero variants and no tag.
>>>
>>> The generator code actually represents both structs and unions as a
>>> common QAPISchemaObjectType already.  QAPI/QMP introspection does the
>>> same: it uses a single meta type 'object' for both.
>>>
>>>
>>> There is another spot where only structs are allowed: a struct or
>>> union's base type.  That restriction will be awkward to lift, as I made
>>> the mistake of baking the assumption "object type has at most one tag
>>> member" into QAPI/QMP introspection .
>> Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained.
>>
>> So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ?
> Permit me a brief digression into history.
>
> The initial QAPI design language provided product types (structs) and
> sum types (unions containing exactly one of several types, and a tag
> member that tells which one).  The two are orthogonal.
>
> These unions turned out rather awkward.
>
> The unions we have today are more general.  They have common members,
> and one of them is the tag member, of enumeration type.  For each tag
> value, they have variant members.  Both the common members and each tag
> value's variant members are given as struct types.
>
> What if the tag's enumeration type is empty, i.e. has no values?  We get
> a union with no variant members, only common ones.  Isn't that a struct?
>
> Not quite.  To get a struct, we also have to drop the tag member.  It
> has no possible values anyway.
>
> You see, struct types are almost a special case of today's union types.
> To overcome "almost", we can introduce the notion of "object type":
>
> * An object type has common members, one of them can be a tag member, of
>    enumeration type, not empty.  For each tag value, it additionally has
>    variant members.
>
> * A union type is an object type with a tag member and variant members.
>
> * A struct type is an object type without tag member and variant
>    members.
>
> The QAPI generator code already made the jump to this object type
> notion.  It transform the special cases into the general case at first
> opportunity, in QAPISchema._def_struct_type() and ._def_union_type().
>
> *Except* we haven't implemented support for variant members in a few
> places where they cannot occur now, e.g. as a tag value's variant.  This
> is the restriction you ran into.
>
> I'd like to make the jump to object type in the QAPI schema language,
> too.  But that's not a prerequisite to lifting the restriction.
>
>> If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ?
> I believe we can implement the missing parts and relax the checks.  But
> to be sure, we need to try.
>
>> or I may have completely misunderstood most of the part 😅. Please let me know
> More questions?

Completely understood everything. Thankyou for the wonderful 
explanation. Looking forward to implement the missing parts in QAPI 
schema language.

Regards,
Het Gala

Re: QAPI unions as branches / unifying struct and union types

[Daniel P. Berrangé]

On Fri, Feb 17, 2023 at 04:48:59PM +0530, Het Gala wrote:
> 
> On 14/02/23 3:46 pm, Markus Armbruster wrote:
> > Het Gala <het.gala@nutanix.com> writes:
> > 
> > > On 10/02/23 12:54 pm, Markus Armbruster wrote:
> > > > Daniel P. Berrangé <berrange@redhat.com> writes:
> > > > 
> > > > [...]
> > > > 
> > > > > > +##
> > > > > > +# @MigrateAddress:
> > > > > > +#
> > > > > > +# The options available for communication transport mechanisms for migration
> > > > > > +#
> > > > > > +# Since 8.0
> > > > > > +##
> > > > > > +{ 'union' : 'MigrateAddress',
> > > > > > +  'base' : { 'transport' : 'MigrateTransport'},
> > > > > > +  'discriminator' : 'transport',
> > > > > > +  'data' : {
> > > > > > +    'socket' : 'MigrateSocketAddr',
> > > > > > +    'exec' : 'MigrateExecAddr',
> > > > > > +    'rdma': 'MigrateRdmaAddr' } }
> > > > > Ideally this would be
> > > > > 
> > > > >      'data' : {
> > > > >        'socket' : 'SocketAddress',
> > > > >        'exec' : 'MigrateCommand',
> > > > >        'rdma': 'InetSocketAddress' } }
> > > > > 
> > > > > though the first SocketAddress isn't possible unless it is easy to
> > > > > lift the QAPI limitation.
> > > > Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
> > > > 
> > > >       scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
> > > >       ../qapi/migration.json: In union 'MigrateAddress':
> > > >       ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
> > > > 
> > > > Emitted by schema.py like this:
> > > > 
> > > >                   if (not isinstance(v.type, QAPISchemaObjectType)
> > > >                           or v.type.variants):
> > > >                       raise QAPISemError(
> > > >                           self.info,
> > > >                           "%s cannot use %s"
> > > >                           % (v.describe(self.info), v.type.describe()))
> > > > 
> > > > This enforces docs/devel/qapi-code-gen.rst's clause
> > > > 
> > > >       The BRANCH's value defines the branch's properties, in particular its
> > > >       type.  The type must a struct type.  [...]
> > > > 
> > > > Next paragraph:
> > > > 
> > > >       In the Client JSON Protocol, a union is represented by an object with
> > > >       the common members (from the base type) and the selected branch's
> > > >       members.  The two sets of member names must be disjoint.
> > > > 
> > > > So, we're splicing in the members of the branch's JSON object.  For that
> > > > to even make sense, the branch type needs to map to a JSON object.  This
> > > > is fundamental.  It's the first part of the condition in the code
> > > > snippet above.
> > > > 
> > > > We have two kinds of QAPI types that map to a JSON object: struct and
> > > > union.  The second part of the condition restricts to struct.  Unless
> > > > I'm missing something (imperfect memory...), this is *not* fundamental,
> > > > just a matter of implementing it.  But I'd have to try to be sure.
> > > > 
> > > > 
> > > > Instead of simply allowing unions in addition to structs here, I'd like
> > > > to go one step further, and fuse the two into "objects".  Let me
> > > > explain.
> > > > 
> > > > If we abstract from syntax, structs have become almost a special kind of
> > > > union.  Unions have a set of common members and sets of variant members,
> > > > and a special common member (the tag) selects the set of variant
> > > > members.  Structs are unions with zero variants and no tag.
> > > > 
> > > > The generator code actually represents both structs and unions as a
> > > > common QAPISchemaObjectType already.  QAPI/QMP introspection does the
> > > > same: it uses a single meta type 'object' for both.
> > > > 
> > > > 
> > > > There is another spot where only structs are allowed: a struct or
> > > > union's base type.  That restriction will be awkward to lift, as I made
> > > > the mistake of baking the assumption "object type has at most one tag
> > > > member" into QAPI/QMP introspection .
> > > Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained.
> > > 
> > > So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ?
> > Permit me a brief digression into history.
> > 
> > The initial QAPI design language provided product types (structs) and
> > sum types (unions containing exactly one of several types, and a tag
> > member that tells which one).  The two are orthogonal.
> > 
> > These unions turned out rather awkward.
> > 
> > The unions we have today are more general.  They have common members,
> > and one of them is the tag member, of enumeration type.  For each tag
> > value, they have variant members.  Both the common members and each tag
> > value's variant members are given as struct types.
> > 
> > What if the tag's enumeration type is empty, i.e. has no values?  We get
> > a union with no variant members, only common ones.  Isn't that a struct?
> > 
> > Not quite.  To get a struct, we also have to drop the tag member.  It
> > has no possible values anyway.
> > 
> > You see, struct types are almost a special case of today's union types.
> > To overcome "almost", we can introduce the notion of "object type":
> > 
> > * An object type has common members, one of them can be a tag member, of
> >    enumeration type, not empty.  For each tag value, it additionally has
> >    variant members.
> > 
> > * A union type is an object type with a tag member and variant members.
> > 
> > * A struct type is an object type without tag member and variant
> >    members.
> > 
> > The QAPI generator code already made the jump to this object type
> > notion.  It transform the special cases into the general case at first
> > opportunity, in QAPISchema._def_struct_type() and ._def_union_type().
> > 
> > *Except* we haven't implemented support for variant members in a few
> > places where they cannot occur now, e.g. as a tag value's variant.  This
> > is the restriction you ran into.
> > 
> > I'd like to make the jump to object type in the QAPI schema language,
> > too.  But that's not a prerequisite to lifting the restriction.
> > 
> > > If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ?
> > I believe we can implement the missing parts and relax the checks.  But
> > to be sure, we need to try.
> > 
> > > or I may have completely misunderstood most of the part 😅. Please let me know
> > More questions?
> 
> Completely understood everything. Thankyou for the wonderful explanation.
> Looking forward to implement the missing parts in QAPI schema language.

I cc'd you on a patch that implements this missing feature a couple
of days ago, and its on Markus' review todo list. So we should be
able to decide how to move forward sometime next week.

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

Re: QAPI unions as branches / unifying struct and union types

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 7271 bytes --]


On 17/02/23 5:25 pm, Daniel P. Berrangé wrote:
> On Fri, Feb 17, 2023 at 04:48:59PM +0530, Het Gala wrote:
>> On 14/02/23 3:46 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com>  writes:
>>>
>>>> On 10/02/23 12:54 pm, Markus Armbruster wrote:
>>>>> Daniel P. Berrangé<berrange@redhat.com>  writes:
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +##
>>>>>>> +# @MigrateAddress:
>>>>>>> +#
>>>>>>> +# The options available for communication transport mechanisms for migration
>>>>>>> +#
>>>>>>> +# Since 8.0
>>>>>>> +##
>>>>>>> +{ 'union' : 'MigrateAddress',
>>>>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>>>>> +  'discriminator' : 'transport',
>>>>>>> +  'data' : {
>>>>>>> +    'socket' : 'MigrateSocketAddr',
>>>>>>> +    'exec' : 'MigrateExecAddr',
>>>>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>>>>> Ideally this would be
>>>>>>
>>>>>>       'data' : {
>>>>>>         'socket' : 'SocketAddress',
>>>>>>         'exec' : 'MigrateCommand',
>>>>>>         'rdma': 'InetSocketAddress' } }
>>>>>>
>>>>>> though the first SocketAddress isn't possible unless it is easy to
>>>>>> lift the QAPI limitation.
>>>>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
>>>>>
>>>>>        scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>>>>>        ../qapi/migration.json: In union 'MigrateAddress':
>>>>>        ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress'
>>>>>
>>>>> Emitted by schema.py like this:
>>>>>
>>>>>                    if (not isinstance(v.type, QAPISchemaObjectType)
>>>>>                            or v.type.variants):
>>>>>                        raise QAPISemError(
>>>>>                            self.info,
>>>>>                            "%s cannot use %s"
>>>>>                            % (v.describe(self.info), v.type.describe()))
>>>>>
>>>>> This enforces docs/devel/qapi-code-gen.rst's clause
>>>>>
>>>>>        The BRANCH's value defines the branch's properties, in particular its
>>>>>        type.  The type must a struct type.  [...]
>>>>>
>>>>> Next paragraph:
>>>>>
>>>>>        In the Client JSON Protocol, a union is represented by an object with
>>>>>        the common members (from the base type) and the selected branch's
>>>>>        members.  The two sets of member names must be disjoint.
>>>>>
>>>>> So, we're splicing in the members of the branch's JSON object.  For that
>>>>> to even make sense, the branch type needs to map to a JSON object.  This
>>>>> is fundamental.  It's the first part of the condition in the code
>>>>> snippet above.
>>>>>
>>>>> We have two kinds of QAPI types that map to a JSON object: struct and
>>>>> union.  The second part of the condition restricts to struct.  Unless
>>>>> I'm missing something (imperfect memory...), this is *not* fundamental,
>>>>> just a matter of implementing it.  But I'd have to try to be sure.
>>>>>
>>>>>
>>>>> Instead of simply allowing unions in addition to structs here, I'd like
>>>>> to go one step further, and fuse the two into "objects".  Let me
>>>>> explain.
>>>>>
>>>>> If we abstract from syntax, structs have become almost a special kind of
>>>>> union.  Unions have a set of common members and sets of variant members,
>>>>> and a special common member (the tag) selects the set of variant
>>>>> members.  Structs are unions with zero variants and no tag.
>>>>>
>>>>> The generator code actually represents both structs and unions as a
>>>>> common QAPISchemaObjectType already.  QAPI/QMP introspection does the
>>>>> same: it uses a single meta type 'object' for both.
>>>>>
>>>>>
>>>>> There is another spot where only structs are allowed: a struct or
>>>>> union's base type.  That restriction will be awkward to lift, as I made
>>>>> the mistake of baking the assumption "object type has at most one tag
>>>>> member" into QAPI/QMP introspection .
>>>> Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained.
>>>>
>>>> So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ?
>>> Permit me a brief digression into history.
>>>
>>> The initial QAPI design language provided product types (structs) and
>>> sum types (unions containing exactly one of several types, and a tag
>>> member that tells which one).  The two are orthogonal.
>>>
>>> These unions turned out rather awkward.
>>>
>>> The unions we have today are more general.  They have common members,
>>> and one of them is the tag member, of enumeration type.  For each tag
>>> value, they have variant members.  Both the common members and each tag
>>> value's variant members are given as struct types.
>>>
>>> What if the tag's enumeration type is empty, i.e. has no values?  We get
>>> a union with no variant members, only common ones.  Isn't that a struct?
>>>
>>> Not quite.  To get a struct, we also have to drop the tag member.  It
>>> has no possible values anyway.
>>>
>>> You see, struct types are almost a special case of today's union types.
>>> To overcome "almost", we can introduce the notion of "object type":
>>>
>>> * An object type has common members, one of them can be a tag member, of
>>>     enumeration type, not empty.  For each tag value, it additionally has
>>>     variant members.
>>>
>>> * A union type is an object type with a tag member and variant members.
>>>
>>> * A struct type is an object type without tag member and variant
>>>     members.
>>>
>>> The QAPI generator code already made the jump to this object type
>>> notion.  It transform the special cases into the general case at first
>>> opportunity, in QAPISchema._def_struct_type() and ._def_union_type().
>>>
>>> *Except* we haven't implemented support for variant members in a few
>>> places where they cannot occur now, e.g. as a tag value's variant.  This
>>> is the restriction you ran into.
>>>
>>> I'd like to make the jump to object type in the QAPI schema language,
>>> too.  But that's not a prerequisite to lifting the restriction.
>>>
>>>> If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ?
>>> I believe we can implement the missing parts and relax the checks.  But
>>> to be sure, we need to try.
>>>
>>>> or I may have completely misunderstood most of the part 😅. Please let me know
>>> More questions?
>> Completely understood everything. Thankyou for the wonderful explanation.
>> Looking forward to implement the missing parts in QAPI schema language.
> I cc'd you on a patch that implements this missing feature a couple
> of days ago, and its on Markus' review todo list. So we should be
> able to decide how to move forward sometime next week.
I did put Daniel's qapi patch and on top, added my patch where I 
introduce 'MigrateAddress' in 'migrate' QAPI, and assigned 
'SocketAddress' union to 'socket' backend transport inside 
'MigrateAddress' union. So iow, union inside union worked smoothly, and 
was able to re-architecture the patchset series accordingly.

> With regards,
> Daniel
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 8536 bytes --]