Re: [PATCH 3/5] migration: Avoid multiple parsing of uri in migration code flow

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

On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote:
> From: Author Het Gala <het.gala@nutanix.com>
> 
> Existing uri is encoded at multiple levels to extract the relevant
> migration information.
> 
> The modified QAPI design maps migration parameters into MigrateChannel
> struct before, thus avoiding double-level uri encoding.
> 
> socket_outgoing_migration() has been depricated as It's only purpose was
> uri parsing.
> Renamed socket_outgoing_migration_internal() as socket_outgoing_migration().
> qemu_uri_parsing() has been introduced to parse uri string (backward
> compatibility) and populate the MigrateChannel struct parameters. Note that
> the function will no longer be needed once the 'uri' parameter is depricated.
> 
> 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/migration.c | 78 +++++++++++++++++++++++++++++++++++--------
>  migration/socket.c    | 15 +--------
>  migration/socket.h    |  3 +-
>  3 files changed, 67 insertions(+), 29 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1b6e62612a..36de9f6a6b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -61,6 +61,7 @@
>  #include "sysemu/cpus.h"
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/sockets.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>  
> @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static void qemu_uri_parsing(const char *uri,
> +                             MigrateChannel **channel,
> +                             Error **errp)

Coding style would prefer 'bool' instad of 'void'...

Also lets call this 'migrate_uri_parse'

> +{
> +    Error *local_err = NULL;
> +    const char *p = NULL;
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> +
> +    if (strstart(uri, "exec:", &p)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        addrs->u.exec.exec_str = g_strdup(p + strlen("exec:"));
> +    } else if (strstart(uri, "rdma:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> +        addrs->u.rdma.rdma_str = g_strdup(p + strlen("rdma:"));
> +    } 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;
> @@ -2397,7 +2431,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)) {
> @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>          }
>      }
>  
> +    /*
> +     * motive here is just to have checks and convert uri into
> +     * socketaddress struct
> +     */
> +    if (uri && channel) {
> +        error_setg(errp, "uri and channels options should be"
> +                          "mutually exclusive");

Needs a 'return' statement after reporting the error. ALso, this
check should be moved to the earlier patch that introduced the
'channel' field.

> +    } else if (uri) {
> +        qemu_uri_parsing(uri, &channel, &local_err);

Needs to 'return' on error, eg

  } else if (uri && !qemu_uri_parsing(...))
      return;

> +    }
> +

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 :|