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

[Het Gala]

On 10/9/2023 7:43 PM, Fabiano Rosas wrote:

> 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
> > > > > > rate_add_address(SocketAddress *address)
> > > > > > ng 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.
>

Got your point perfectly now. Thanks a lot. Just so that I understood right, summarising it in short:

g_autoptr --> helps in error paths so we use it, but for non-error paths if it goes out of scope, it will free it so to prevent that we have to use g_steal_pointer(). Either use g_autoptr/g_steal_pointer pair or do it simple but then will have to take care of error paths as well as non-error paths to free the pointers.

> > I think the syntax follows as the second example given here: 
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.gtk.org_glib_func.steal-5Fpointer.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=Fn2ZhrrsSvcV_ZUX3PUnI1zMMD7JM5o1X9LbURlU1B7O06RNnzmOS3hKI4HBe6fB&s=iJFFbZnxzfqd1xu3gRVfvdjjE_X3XT3t0aRRAIeaUUc&e=  ?
> >       
> Yep, that's it.
>

Ack.

    Regards,
    Het Gala