Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

["Daniel P. Berrangé" <berrange@redhat.com>]

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.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@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 :|