Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.

[Het Gala]

On 10/9/2023 8:05 PM, Fabiano Rosas wrote:

> Het Gala <het.gala@nutanix.com> writes:
>
> > On 10/4/2023 8:55 PM, Fabiano Rosas wrote:
> > > Het Gala<het.gala@nutanix.com>  writes:
> > >
> > > > Integrate MigrateChannelList with all transport backends
> > > > (socket, exec and rdma) for both src and dest migration
> > > > endpoints for hmp migration.
> > > >
> > > > 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/migration-hmp-cmds.c | 15 +++++++++++++--
> > > >   migration/migration.c          |  5 ++---
> > > >   migration/migration.h          |  3 ++-
> > > >   3 files changed, 17 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> > > > index a2e6a5c51e..a1657f3d37 100644
> > > > --- a/migration/migration-hmp-cmds.c
> > > > +++ b/migration/migration-hmp-cmds.c
> > > > @@ -441,9 +441,14 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
> > > >   {
> > > >       Error *err = NULL;
> > > >       const char *uri = qdict_get_str(qdict, "uri");
> > > > +    MigrationChannelList *caps = NULL;
> > > > +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> > > >           
> > > Just the pointer here. If I remember correctly the g_autoptr here would
> > > cause a double free when freeing the caps.
> > >         
> > Yes, we'll just have 'g_autoptr(MigrationChannel) channel = NULL'.
> >
> > Is it because inside QAPI_LIST_PREPEND, caps will be refrencing to the 
> > same memory as 'channel', we don't need to free channel ?
> >       
> Slightly different scenario here. Here the issue is that we will free
> the caps with qapi_free_MigrationChannel() before returning. Then, at
> function exit the g_autoptr will try to free 'channel', which has
> already been freed along with 'caps'. That's a double free, I think it
> hits an assert inside glib, if I remember correctly.
>
> > I am still not 
> > sure what is the right place to use g_steal_pointer(), is this a right 
> > place to use (non-error paths) ?
> >       
> It doesn't look like we need it here. As long as the qapi list has a
> reference and we're freeing the caps, then channel should be freed by
> that function already.
>     
    Ack. Yes, with the discussion in earlier patches, I also don't think
    we need g_autoptr too here. Normal pointer is enough as we are
    freeing the memory after the function is returned.

> > > >   
> > > > -    qmp_migrate_incoming(uri, false, NULL, &err);
> > > > +    migrate_uri_parse(uri, &channel, &err);
> > > > +    QAPI_LIST_PREPEND(caps, channel);
> > > >   
> > > > +    qmp_migrate_incoming(NULL, true, caps, &err);
> > > > +    qapi_free_MigrationChannelList(caps);
> > > >       hmp_handle_error(mon, err);
> > > >   }
> > > >   
> > > > @@ -730,9 +735,15 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
> > > >       bool resume = qdict_get_try_bool(qdict, "resume", false);
> > > >       const char *uri = qdict_get_str(qdict, "uri");
> > > >       Error *err = NULL;
> > > > +    MigrationChannelList *caps = NULL;
> > > > +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> > > >           
> > > Same here. We free the channel via caps and we attribute it below, no
> > > need to allocate.
> > >         
> > Ack.
> > > >   
> > > > -    qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc,
> > > > +    migrate_uri_parse(uri, &channel, &err);
> > > > +    QAPI_LIST_PREPEND(caps, channel);
> > > > +
> > > > +    qmp_migrate(NULL, true, caps, !!blk, blk, !!inc, inc,
> > > >                    false, false, true, resume, &err);
> > > > +    qapi_free_MigrationChannelList(caps);
> > > >       if (hmp_handle_error(mon, err)) {
> > > >           
> > >         
> > Regards,
> > Het Gala
> >       
>     
    Regards,
    Het Gala