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

[Fabiano Rosas <farosas@suse.de>]

Het Gala <het.gala@nutanix.com> writes:

> On 10/4/2023 11:42 PM, Fabiano Rosas wrote:
>> 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.
> Okay. I get the point that g_autoptr helps to deal with freeing of 
> pointer in case error occurs inside the function.

Yes, but note g_autoptr will free the memory any time the variable goes
out of scope, i.e. any time we return from the function. Even when
there's no error and we actually want that memory to still be around for
the callers to use.

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

So now 'channel' points to the memory you allocated at the start of the
function with g_new0. When the function returns, g_autoptr has no idea
that someone is still using that memory, so it will just free it.

Whenever you want a chunk of memory to be accessed across function calls
like that you need to steal the reference. After stealing, the pointer
that was registered with g_autoptr (in this case 'addr') now points to
nothing and the pointer that was returned/assigned is a different one
that isn't known by any cleanup functions.

Note that after g_steal_pointer, that memory now becomes responsibility
of whatever piece of code owns that pointer. In this case,
qemu_start_incoming_migration() and qmp_migrate(). Those two functions
will have to free the memory once they are done with it. Or do the
g_autoptr/g_steal_pointer trick once more.

> I think the syntax follows as the second example given here: 
> https://docs.gtk.org/glib/func.steal_pointer.html ?

Yep, that's it.