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

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 04, 2023 at 11:43:12AM -0300, 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.
>
> 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.