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

[Het Gala]

On 10/4/2023 11:42 PM, Fabiano Rosas wrote:
Daniel P. Berrangé<berra...@redhat.com>  writes:
On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
Het Gala<het.g...@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.retnaka...@nutanix.com>
Signed-off-by: Het Gala<het.g...@nutanix.com>
Reviewed-by: Daniel P. Berrangé<berra...@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.
It is still good to use g_autoptr to deal with the error paths.

On the success path though you need   g_steal_pointer(&addr) to
prevent the autofree cleanup running.
Ah good point, this has been suggested in an earlier version already, I
forgot to mention. We should definitely use g_steal_pointer() whenever
the variable goes out of scope.
Okay. I get the point that g_autoptr helps to deal with freeing of 
pointer in case error occurs inside the function.
But I am still trying to figure out why we need g_steal_pointer() ? If 
you could please explain once again.
My understanding till now was that if we want to return 'addr' variable 
as return type, then we would want to make use of g_steal_pointer(&addr) 
so instead of addr, we pass a temp ptr refrencing to the same location 
as addr, and then assign addr = NULL. But we are already assigning the 
memory location where addr was refrencing to 'channel'. Let me know if I 
am missing something ?
I think the syntax follows as the second example given here: 
https://docs.gtk.org/glib/func.steal_pointer.html ?


Regards,
Het Gala