Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Het Gala]

On 10/4/2023 8:13 PM, Fabiano Rosas wrote:

> Het Gala <het.gala@nutanix.com> writes:
>
> > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> > string containing migration connection related information
> > and stores them inside well defined 'MigrateAddress' struct.
> >
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  migration/exec.c      |  1 -
> >  migration/exec.h      |  4 ++++
> >  migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/exec.c b/migration/exec.c
> > index 2bf882bbe1..32f5143dfd 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);
> > 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 6d3cf5d5cd..dcbd509d56 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -65,6 +65,7 @@
> >  #include "sysemu/qtest.h"
> >  #include "options.h"
> >  #include "sysemu/dirtylimit.h"
> > +#include "qemu/sockets.h"
> >  
> >  static NotifierList migration_state_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
> >                        QAPI_CLONE(SocketAddress, address));
> >  }
> >  
> > +static bool migrate_uri_parse(const char *uri,
> > +                              MigrationAddress **channel,
> > +                              Error **errp)
> > +{
> > +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> >       
> This cannot be g_autoptr because you're passing it out of scope at the
> end of the function.
>     
> > +    SocketAddress *saddr = &addr->u.socket;
> >       
> This attribution is useless. Down below you overwrite it with the result
> of socket_parse.
>     
    Ack. Will assign saddr to NULL here.

> > +    InetSocketAddress *isock = &addr->u.rdma;
> > +    strList **tail = &addr->u.exec.args;
> > +
> > +    if (strstart(uri, "exec:", NULL)) {
> > +        addr->transport = MIGRATION_ADDRESS_TYPE_EXEC;
> > +#ifdef WIN32
> > +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> > +        QAPI_LIST_APPEND(tail, g_strdup("/c"));
> > +#else
> > +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> > +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> > +#endif
> > +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> > +    } else if (strstart(uri, "rdma:", NULL)) {
> > +        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > +            qapi_free_InetSocketAddress(isock);
> > +            return false;
> > +        }
> > +        addr->transport = MIGRATION_ADDRESS_TYPE_RDMA;
> > +    } else if (strstart(uri, "tcp:", NULL) ||
> > +                strstart(uri, "unix:", NULL) ||
> > +                strstart(uri, "vsock:", NULL) ||
> > +                strstart(uri, "fd:", NULL)) {
> > +        addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> > +        saddr = socket_parse(uri, errp);
> > +        if (!saddr) {
> > +            qapi_free_SocketAddress(saddr);
> >       
> Shouldn't free here. socket_parse() already does so on failure.
>     
    Okay, understood. So should just return false, nothing else here.

> > +            return false;
> > +        }
> >       
> Then here you can set the values you intended to set.
>
> addr->u.socket.type = saddr->type;
> addr->u.socket.u = saddr->u;
>     
    Ack. Will do that change.

> > +    } else {
> > +        error_setg(errp, "unknown migration protocol: %s", uri);
> > +        return false;
> > +    }
> > +
> > +    *channel = addr;
> > +    return true;
> > +}
> > +
> >  static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >  {
> >      const char *p = NULL;
> > +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> >       
> The memory is leaked here because the pointer is overwritten below. Just
> set it to NULL. You can keep the g_autoptr.
>     
    Ack. You mean it is overwritten by migrate_parse_uri function.

> >  
> >      /* URI is not suitable for migration? */
> >      if (!migration_channels_and_uri_compatible(uri, errp)) {
> >          return;
> >      }
> >  
> > +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> > +        return;
> > +    }
> > +
> >      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> >      if (strstart(uri, "tcp:", &p) ||
> >          strstart(uri, "unix:", NULL) ||
> > @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >      Error *local_err = NULL;
> >      MigrationState *s = migrate_get_current();
> >      const char *p = NULL;
> > +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> >       
> Same here.
>     
    Ack. Will change that.

> >  
> >      /* URI is not suitable for migration? */
> >      if (!migration_channels_and_uri_compatible(uri, errp)) {
> >          return;
> >      }
> >  
> > +    if (!migrate_uri_parse(uri, &channel, errp)) {
> > +        return;
> > +    }
> > +
> >      resume_requested = has_resume && resume;
> >      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> >                           resume_requested, errp)) {
> >       
>     
    Regards,
    Het Gala