| Message ID | 20231116063448.2333616-1-zhouzongmin@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] migration: free 'saddr' since be no longer used | expand |
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? As Peter said, putting a comment why we don't use qapi_free_SocketAddress() will be a good idea. Later, Juan.
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. >
On Fri, Nov 17, 2023 at 10:51:18AM +0800, Zongmin Zhou wrote: > > 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 Normally we use "comment" to represent direct comment in the code. You explained it in the "commit message". :) That explanation is good enough to me, you can add a summary comment in the code too. Something like: /* Don't free the objects inside; their ownership moved to "addr" */
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:"));
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(+)