On 01.07.20 16:38, Eric Blake wrote: > On 6/30/20 3:45 AM, Max Reitz wrote: >> The created structure is not really a proper QAPI object, so we cannot >> and will not free its members. Strings therein should therefore not be >> duplicated, or we will leak them. > > This seems fragile to me; having to code QAPI usage differently > depending on whether the containing struct was malloc'd or not (and > therefore whether someone will call qapi_free_MigrateSetParameters or > not) looks awkward to maintain.
I don’t think that’s the point. The point is that it’s just a temporary
object to run some check function on. This is a very... special use case.
> We have
> visit_type_MigrateSetParameters_members, could that be used as a cleaner
> way to free all members of the struct without freeing the struct itself?
> Should the QAPI generator start generating qapi_free_FOO_members to
> make such cleanup easier?
The whole code is a mess, in my opinion.
The real question is why don’t we just drop migrate_params_test_apply()
and let qmp_migrate_set_parameters() invoke migrate_params_check()
directly on @params.
I think there was some reason why I didn’t do that, but unfortunately I
don’t remember it off the top of my head (if there was a reason).
In any case, I don’t think any of this is the QAPI generator’s fault.
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>> migration/migration.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 481a590f72..47c7da4e55 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1336,12 +1336,12 @@ static void
>> migrate_params_test_apply(MigrateSetParameters *params,
>> if (params->has_tls_creds) {
>> assert(params->tls_creds->type == QTYPE_QSTRING);
>> - dest->tls_creds = g_strdup(params->tls_creds->u.s);
>> + dest->tls_creds = params->tls_creds->u.s;
>> }
>> if (params->has_tls_hostname) {
>> assert(params->tls_hostname->type == QTYPE_QSTRING);
>> - dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
>> + dest->tls_hostname = params->tls_hostname->u.s;
>> }
>> if (params->has_max_bandwidth) {
>>
>
signature.asc
Description: OpenPGP digital signature
