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

2020-07-02 Thread Max Reitz
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 
>> ---
>>   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


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

2020-07-01 Thread Eric Blake

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.  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?




Signed-off-by: Max Reitz 
---
  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) {




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




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

2020-07-01 Thread Vladimir Sementsov-Ogievskiy

30.06.2020 11:45, 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.

Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir



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

2020-06-30 Thread Dr. David Alan Gilbert
* Max Reitz (mre...@redhat.com) 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.
> 
> Signed-off-by: Max Reitz 
> ---
>  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;
>  }

Yeh I think I agree.

Reviewed-by: Dr. David Alan Gilbert 

>  
>  if (params->has_max_bandwidth) {
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK




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

2020-06-30 Thread Max Reitz
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.

Signed-off-by: Max Reitz 
---
 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) {
-- 
2.26.2