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

[Het Gala]

On 09/01/23 7:44 pm, Daniel P. Berrangé wrote:

On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote:

From: Author Het Gala <het.g...@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.

@@ -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'

Sure, Ack. Will change it in the upcoming patch.

+    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;'

Ack.

+}
+
  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.

Okay sure. Will introduce the check in 2nd patch itself.

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

Needs to 'return' on error, eg

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

Ack.

With regards,
Daniel

Regards,
Het Gala