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.3On 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 :|
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
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 :|
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
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 :|
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
© 2016 - 2026 Red Hat, Inc.