Re: [PATCH v6 08/13] migration: Export tls-[creds|hostname|authz] params to cmdline too

[Daniel P . Berrangé]

On Tue, May 17, 2022 at 03:57:25PM -0400, Peter Xu wrote:
> It's useful for specifying tls credentials all in the cmdline (along with
> the -object tls-creds-*), especially for debugging purpose.
> 
> The trick here is we must remember to not free these fields again in the
> finalize() function of migration object, otherwise it'll cause double-free.
> 
> The thing is when destroying an object, we'll first destroy the properties
> that bound to the object, then the object itself.  To be explicit, when
> destroy the object in object_finalize() we have such sequence of
> operations:
> 
> object_property_del_all(obj);
> object_deinit(obj, ti);
> 
> So after this change the two fields are properly released already even
> before reaching the finalize() function but in object_property_del_all(),
> hence we don't need to free them anymore in finalize() or it's double-free.
> 
> This also fixes a trivial memory leak for tls-authz as we forgot to free it
> before this patch.
> 
> Signed-off-by: Peter Xu 
> ---
>  migration/migration.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH v6 08/13] migration: Export tls-[creds|hostname|authz] params to cmdline too

[Peter Xu]

It's useful for specifying tls credentials all in the cmdline (along with
the -object tls-creds-*), especially for debugging purpose.

The trick here is we must remember to not free these fields again in the
finalize() function of migration object, otherwise it'll cause double-free.

The thing is when destroying an object, we'll first destroy the properties
that bound to the object, then the object itself.  To be explicit, when
destroy the object in object_finalize() we have such sequence of
operations:

object_property_del_all(obj);
object_deinit(obj, ti);

So after this change the two fields are properly released already even
before reaching the finalize() function but in object_property_del_all(),
hence we don't need to free them anymore in finalize() or it's double-free.

This also fixes a trivial memory leak for tls-authz as we forgot to free it
before this patch.

Signed-off-by: Peter Xu 
---
 migration/migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d17f435d08..aa4185148c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4379,6 +4379,9 @@ static Property migration_properties[] = {
   DEFAULT_MIGRATE_ANNOUNCE_STEP),
 DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState,
   postcopy_preempt_break_huge, true),
+DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
+DEFINE_PROP_STRING("tls-hostname", MigrationState, 
parameters.tls_hostname),
+DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
 
 /* Migration capabilities */
 DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -4412,12 +4415,9 @@ static void migration_class_init(ObjectClass *klass, 
void *data)
 static void migration_instance_finalize(Object *obj)
 {
 MigrationState *ms = MIGRATION_OBJ(obj);
-MigrationParameters *params = >parameters;
 
 qemu_mutex_destroy(>error_mutex);
 qemu_mutex_destroy(>qemu_file_lock);
-g_free(params->tls_hostname);
-g_free(params->tls_creds);
 qemu_sem_destroy(>wait_unplug_sem);
 qemu_sem_destroy(>rate_limit_sem);
 qemu_sem_destroy(>pause_sem);
-- 
2.32.0