[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply
From: |
Max Reitz |
Subject: |
Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply |
Date: |
Thu, 2 Jul 2020 10:14:25 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
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) {
>>
>
signature.asc
Description: OpenPGP digital signature