On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> Socket transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for socket connection into well defined SocketAddress struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com>
> Signed-off-by: Het Gala <het.g...@nutanix.com>
> ---
> migration/exec.c | 4 ++--
> migration/exec.h | 4 ++++
> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
> migration/socket.c | 34 +++++----------------------------
> migration/socket.h | 7 ++++---
> 5 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -27,7 +27,6 @@
> #include "qemu/cutils.h"
>
> #ifdef WIN32
> -const char *exec_get_cmd_path(void);
> const char *exec_get_cmd_path(void)
> {
> g_autofree char *detected_path = g_new(char, MAX_PATH);
> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> }
> #endif
>
> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
> Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> + Error **errp)
> {
> QIOChannel *ioc;
>
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..736cd71028 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,6 +19,10 @@
>
> #ifndef QEMU_MIGRATION_EXEC_H
> #define QEMU_MIGRATION_EXEC_H
> +
> +#ifdef WIN32
> +const char *exec_get_cmd_path(void);
> +#endif
> void exec_start_incoming_migration(const char *host_port, Error **errp);
>
> void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> diff --git a/migration/migration.c b/migration/migration.c
> index a7e4e286aa..61f52d2f90 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>
> if (strstart(uri, "exec:", NULL)) {
> addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +#ifdef WIN32
> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> +#else
> QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +#endif
This windows portability code should have been in the previous patch
I think.
> QAPI_LIST_APPEND(tail, g_strdup("-c"));
> QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> } else if (strstart(uri, "rdma:", NULL) &&
> @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
>
> static void qemu_start_incoming_migration(const char *uri, Error **errp)
> {
> + Error *local_err = NULL;
> const char *p = NULL;
> MigrateAddress *channel = g_new0(MigrateAddress, 1);
> + SocketAddress *saddr;
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char
> *uri, Error **errp)
> goto out;
> }
>
> + saddr = &channel->u.socket;
Accessing u.socket before checkout transport == SOCKET is bad
practice, even though this is technically safe.
> qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> - if (strstart(uri, "tcp:", &p) ||
> - strstart(uri, "unix:", NULL) ||
> - strstart(uri, "vsock:", NULL)) {
> - socket_start_incoming_migration(p ? p : uri, errp);
> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
THis should have
SocketAddress *saddr = &channe->u.socket
so that 'saddr' is limited in scope to where we've validated
transport == SOCKET
> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> + socket_start_incoming_migration(saddr, &local_err);
> + } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> + fd_start_incoming_migration(saddr->u.fd.str, &local_err);
> + }
> #ifdef CONFIG_RDMA
> } else if (strstart(uri, "rdma:", &p)) {
> rdma_start_incoming_migration(p, 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 {
> error_setg(errp, "unknown migration protocol: %s", uri);
> }
>
> + if (local_err) {
> + qapi_free_SocketAddress(saddr);
> + error_propagate(errp, local_err);
> + return;
THis leaks 'channel', and free's 'saddr' which actually belongs
to channel.
With my comments on the previous patch suggesting g_autoptr for
'channel', we don't need any free calls for 'saddr' or 'channel'.
> + }
> +
> out:
> qapi_free_MigrateAddress(channel);
> }
> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool
> blk,
> MigrationState *s = migrate_get_current();
> const char *p = NULL;
> MigrateAddress *channel = g_new0(MigrateAddress, 1);
> + SocketAddress *saddr;
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool
> blk,
> }
> }
>
> - if (strstart(uri, "tcp:", &p) ||
> - strstart(uri, "unix:", NULL) ||
> - strstart(uri, "vsock:", NULL)) {
> - socket_start_outgoing_migration(s, p ? p : uri, &local_err);
> + saddr = &channel->u.socket;
Again, put this *after* checking transport == SOCKET
> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> + 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 (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);
> } else {
> if (!(has_resume && resume)) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool
> blk,
> if (!(has_resume && resume)) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> }
> + qapi_free_SocketAddress(saddr);
This saddr pointer belongs to 'channel' which must be freed.
> migrate_fd_error(s, local_err);
> error_propagate(errp, local_err);
> return;
> diff --git a/migration/socket.c b/migration/socket.c
> index 1b6f5baefb..8e7430b266 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -108,10 +108,9 @@ out:
> object_unref(OBJECT(sioc));
> }
>
> -static void
> -socket_start_outgoing_migration_internal(MigrationState *s,
> - SocketAddress *saddr,
> - Error **errp)
> +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);
> @@ -135,18 +134,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)
> @@ -172,9 +159,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();
> @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress
> *saddr,
> }
> }
>
> -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 dc54df4e6c..5e4c33b8ea 100644
> --- a/migration/socket.h
> +++ b/migration/socket.h
> @@ -19,13 +19,14 @@
>
> #include "io/channel.h"
> #include "io/task.h"
> +#include "qemu/sockets.h"
>
> 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, const char *str,
> - Error **errp);
> +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 :|
