Re: [PATCH v2] migration: free 'saddr' since be no longer used

[Zongmin Zhou]

On 2023/11/16 22:19, Juan Quintela wrote:
> Zongmin Zhou <zhouzongmin@kylinos.cn> wrote:
> > Since socket_parse() will allocate memory for 'saddr',and its value
> > will pass to 'addr' that allocated by migrate_uri_parse(),
> > then 'saddr' will no longer used,need to free.
> > But due to 'saddr->u' is shallow copying the contents of the union,
> > the members of this union containing allocated strings,and will be used after 
> > that.
> > So just free 'saddr' itself without doing a deep free on the contents of the 
> > SocketAddress.
> >
> > Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'")
> > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > ---
> >   migration/migration.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 28a34c9068..9bdbcdaf49 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -493,6 +493,7 @@ bool migrate_uri_parse(const char *uri, MigrationChannel 
> > **channel,
> >           }
> >           addr->u.socket.type = saddr->type;
> >           addr->u.socket.u = saddr->u;
> > +        g_free(saddr);
> >       } else if (strstart(uri, "file:", NULL)) {
> >           addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
> >           addr->u.file.filename = g_strdup(uri + strlen("file:"));
> Once that we are here, can we move the declaration of saddr to this
> block, so we are sure that we don't use saddr anywhere?
do you mean to do the changes below at this block?
SocketAddress *saddr = socket_parse(uri, errp);
That sounds good and make it clear that 'saddr' is only used on this block.
> As Peter said, putting a comment why we don't use
> qapi_free_SocketAddress() will be a good idea.

I have put some comments on patch v2 to explain

why just free 'saddr' itself without doing a deep free on the contents 
of the SocketAddress .

Maybe need to explicit clarify why g_free is used instead of 
qapi_free_SocketAddress()?


Best regards!

> Later, Juan.