Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Daniel P . Berrangé]

On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> This patch introduces code that can parse 'uri' string parameter and
> spit out 'MigrateAddress' struct. All the required migration parameters
> are stored in the struct.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ee07802a5..a7e4e286aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -64,6 +64,7 @@
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
>  #include "options.h"
> +#include "qemu/sockets.h"
>  
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrateAddress **channel,
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
> +    InetSocketAddress *isock = &addrs->u.rdma;
> +    strList **tail = &addrs->u.exec.args;
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> +    } else if (strstart(uri, "rdma:", NULL) &&
> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;

I would have this as 

    } else if (strstart(uri, "rdma:", NULL)) {
        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
            addrs->transport = MIGRATE_TRANSPORT_RDMA;
        }

as IMHO it is bad practice to have control pass to the next
else if clause when inet_parse() fails, as we know this is
only an RDMA addr

Also you need to use '&local_err' not 'errp' in the inet_parse
call, otherwise the later code block for cleanup won't run.


> +    } 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 = *saddr;

Protect with

   if (saddr != NULL) {
       addrs->u.socket = *saddr;
   }

> +    }
> +
> +    if (local_err) {
> +        qapi_free_MigrateAddress(addrs);
> +        qapi_free_SocketAddress(saddr);
> +        qapi_free_InetSocketAddress(isock);
> +        error_propagate(errp, local_err);
> +        return false;
> +    }
> +
> +    *channel = addrs;
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);

Avoid the later 'out:' cleanup block by using:

  g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> +        error_setg(errp, "Error parsing uri");

This error_setg() is overwriting the error migrate_uri_parse already
set, so just drop it.

> +        goto out;
>      }
>  
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char 
> *uri, Error **errp)
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);
>  }
>  
>  static void process_incoming_migration_bh(void *opaque)
> @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);

Here too, use g_autoptr(MigrateAddress) and drop the  'out:' block

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
> +        error_setg(errp, "Error parsing uri");
> +        goto out;
>      }
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);
> +    return;
>  }
>  
>  void qmp_migrate_cancel(Error **errp)
> -- 
> 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 :|