Expecting every pointer member from s->parameters to be freed
individually is prone to leave some fields forgotten when the code is
eventually updated. Use a dealloc visitor to free them all at once.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 733aae51a8..cc5a66750c 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
     return &ap;
 }
 
-static void migrate_tls_opts_free(MigrationParameters *params)
+static bool migrate_params_free(MigrationParameters *params, Error **errp)
 {
-    qapi_free_StrOrNull(params->tls_creds);
-    qapi_free_StrOrNull(params->tls_hostname);
-    qapi_free_StrOrNull(params->tls_authz);
+    Visitor *v = qapi_dealloc_visitor_new();
+    bool ret;
+
+    /*
+     * qapi_free_MigrationParameters can't be used here because
+     * MigrationParameters is embedded in MigrationState due to qdev
+     * needing to access the offset of the migration properties inside
+     * the migration object.
+     */
+    ret = visit_type_MigrationParameters_members(v, params, errp);
+    visit_free(v);
+
+    return ret;
 }
 
 /* normalize QTYPE_QNULL to QTYPE_QSTRING "" */
@@ -1290,9 +1300,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
         return;
     }
 
-    migrate_tls_opts_free(cur);
-    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
-    qapi_free_strList(cur->cpr_exec_command);
+    if (!migrate_params_free(cur, errp)) {
+        return;
+    }
 
     /* mark all present, so they're all copied */
     migrate_mark_all_params_present(tmp);
-- 
2.51.0

On Mon, Feb 02, 2026 at 07:41:00PM -0300, Fabiano Rosas wrote:
> Expecting every pointer member from s->parameters to be freed
> individually is prone to leave some fields forgotten when the code is
> eventually updated. Use a dealloc visitor to free them all at once.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 733aae51a8..cc5a66750c 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
>      return &ap;
>  }
>  
> -static void migrate_tls_opts_free(MigrationParameters *params)
> +static bool migrate_params_free(MigrationParameters *params, Error **errp)
>  {
> -    qapi_free_StrOrNull(params->tls_creds);
> -    qapi_free_StrOrNull(params->tls_hostname);
> -    qapi_free_StrOrNull(params->tls_authz);
> +    Visitor *v = qapi_dealloc_visitor_new();
> +    bool ret;
> +
> +    /*
> +     * qapi_free_MigrationParameters can't be used here because
> +     * MigrationParameters is embedded in MigrationState due to qdev
> +     * needing to access the offset of the migration properties inside
> +     * the migration object.
> +     */

Ohhhhhhhh.... thanks for the comment!

Reviewed-by: Peter Xu <peterx@redhat.com>

> +    ret = visit_type_MigrationParameters_members(v, params, errp);
> +    visit_free(v);
> +
> +    return ret;
>  }

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Feb 02, 2026 at 07:41:00PM -0300, Fabiano Rosas wrote:
>> Expecting every pointer member from s->parameters to be freed
>> individually is prone to leave some fields forgotten when the code is
>> eventually updated. Use a dealloc visitor to free them all at once.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 733aae51a8..cc5a66750c 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
>>      return &ap;
>>  }
>>  
>> -static void migrate_tls_opts_free(MigrationParameters *params)
>> +static bool migrate_params_free(MigrationParameters *params, Error **errp)
>>  {
>> -    qapi_free_StrOrNull(params->tls_creds);
>> -    qapi_free_StrOrNull(params->tls_hostname);
>> -    qapi_free_StrOrNull(params->tls_authz);
>> +    Visitor *v = qapi_dealloc_visitor_new();
>> +    bool ret;
>> +
>> +    /*
>> +     * qapi_free_MigrationParameters can't be used here because
>> +     * MigrationParameters is embedded in MigrationState due to qdev
>> +     * needing to access the offset of the migration properties inside
>> +     * the migration object.
>> +     */
>
> Ohhhhhhhh.... thanks for the comment!
>

There's some amount of rigidness caused by qdev requirements
unfortunately.

I'm not sure if I ever posted it, but I wrote some code to move the
parameters into a MigrationOptions object so we could make
MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
something like -global migration-options by default, so it kinda killed
the idea.

> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>> +    ret = visit_type_MigrationParameters_members(v, params, errp);
>> +    visit_free(v);
>> +
>> +    return ret;
>>  }

On Fri, Feb 06, 2026 at 09:29:04AM -0300, Fabiano Rosas wrote:
> There's some amount of rigidness caused by qdev requirements
> unfortunately.
> 
> I'm not sure if I ever posted it, but I wrote some code to move the
> parameters into a MigrationOptions object so we could make
> MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
> something like -global migration-options by default, so it kinda killed
> the idea.

It's not strictly about TYPE_DEVICE, but reusing of qdev properties, right?
At least the issue described by your comment was about offseting and it
sounds like so.

I just want to double check with you that I think the problem you described
will also present even after applying my other series:

https://lore.kernel.org/r/20251209162857.857593-1-peterx@redhat.com

That series only removes the TYPE_DEVICE dependency, but not qdev
properties.  I think it's the qdev property trick that is relevant at least
to the offset issue you mentioned so MigrationParameters cannot be
g_new()ed.

The major use case for this qdev reuse is: (1) help scripting, so as to use
-global migration.XXX=YYY, (2) support migration in machine compat
properties.  IIUC (1) isn't a major thing we ask for (again, maybe I used
the most of it.. but maybe only me; I'm not sure..), as long as anything
can keep (2) working then we can consider.

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 06, 2026 at 09:29:04AM -0300, Fabiano Rosas wrote:
>> There's some amount of rigidness caused by qdev requirements
>> unfortunately.
>> 
>> I'm not sure if I ever posted it, but I wrote some code to move the
>> parameters into a MigrationOptions object so we could make
>> MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
>> something like -global migration-options by default, so it kinda killed
>> the idea.
>
> It's not strictly about TYPE_DEVICE, but reusing of qdev properties, right?
> At least the issue described by your comment was about offseting and it
> sounds like so.
>

Absolutely, I was just rambling.

Of course, TYPE_DEVICE brings us weirdness such as:

(qemu) device_add migration,help                             
migration options:                                           
  announce-initial=<size> -  (default: 50)         
  announce-max=<size>    -  (default: 550)        
  announce-rounds=<size> -  (default: 5)    
...

So it would be nice to drop this dependency.

> I just want to double check with you that I think the problem you described
> will also present even after applying my other series:
>
> https://lore.kernel.org/r/20251209162857.857593-1-peterx@redhat.com
>
> That series only removes the TYPE_DEVICE dependency, but not qdev
> properties.  I think it's the qdev property trick that is relevant at least
> to the offset issue you mentioned so MigrationParameters cannot be
> g_new()ed.
>
> The major use case for this qdev reuse is: (1) help scripting, so as to use
> -global migration.XXX=YYY, (2) support migration in machine compat
> properties.  IIUC (1) isn't a major thing we ask for (again, maybe I used
> the most of it.. but maybe only me; I'm not sure..), as long as anything
> can keep (2) working then we can consider.

The properties are also useful in providing defaults for the migration
options and perhaps we could make use of the .get/.set methods in a more
convenient way in the migration code, such as implementing per-option
input validation instead of checking all options always. (although I
haven't thought about the overlap with QAPI)

About -global, we should do better to separate the compat options from
the regular options. (and no, a blank line in options.c is not enough
separation!). I worry someday we'll break something in compat while
trying to change the regular options.

Another point is that even after all the recent changes to options.c, we
are still left with a list of migration_properties that is optional to
use. Which means setting defaults is also optional.

If we decide to retroactively add a default we'll suddenly have a new
option showing up in -global! Having to discover mid-way through the
implementation of a new migration feature that setting a default like
the other options do will also add property code to your option and now
you have one more source of SIGSEGV is not super fun either.

I had the intention to somehow force every option to have an equivalent
in migration_properties so that every option would have a default
explicitly set, but seeing the recent work on fixing the StrOrNull
property, I don't think it's worth it to ask that people go through the
hassle of implementing a property (when the new option cannot use the
existing types).