Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2423 bytes --]

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 <mreitz@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) {
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]