[Qemu-devel] [PATCH] migration: fix small leaks

[Qemu-devel] [PATCH] migration: fix small leaks

[Marc-André Lureau]

Spotted thanks to valgrind and tests/device-introspect-test:

==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
==11711==    by 0x695693: migration_instance_init (migration.c:2226)
==11711==    by 0x717C4B: object_init_with_type (object.c:344)
==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
==11711==    by 0x7182EB: object_new_with_type (object.c:483)
==11711==    by 0x718328: object_new (object.c:493)
==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/migration.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 085c32c994..c3fe0ed9ca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
     dc->props = migration_properties;
 }
 
+static void migration_instance_finalize(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+    MigrationParameters *params = &ms->parameters;
+
+    g_free(params->tls_hostname);
+    g_free(params->tls_creds);
+}
+
 static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
@@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
     .class_size = sizeof(MigrationClass),
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
+    .instance_finalize = migration_instance_finalize,
 };
 
 static void register_migration_types(void)
-- 
2.14.0.rc0.1.g40ca67566

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Peter Xu]

On Tue, Aug 01, 2017 at 05:04:18PM +0100, Marc-André Lureau wrote:
> Spotted thanks to valgrind and tests/device-introspect-test:
> 
> ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> ==11711==    by 0x718328: object_new (object.c:493)
> ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Juan Quintela]

Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Spotted thanks to valgrind and tests/device-introspect-test:
>
> ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> ==11711==    by 0x718328: object_new (object.c:493)
> ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Spotted thanks to valgrind and tests/device-introspect-test:
> 
> ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> ==11711==    by 0x718328: object_new (object.c:493)
> ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Queued for migration

> ---
>  migration/migration.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 085c32c994..c3fe0ed9ca 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
>      dc->props = migration_properties;
>  }
>  
> +static void migration_instance_finalize(Object *obj)
> +{
> +    MigrationState *ms = MIGRATION_OBJ(obj);
> +    MigrationParameters *params = &ms->parameters;
> +
> +    g_free(params->tls_hostname);
> +    g_free(params->tls_creds);
> +}
> +
>  static void migration_instance_init(Object *obj)
>  {
>      MigrationState *ms = MIGRATION_OBJ(obj);
> @@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
>      .class_size = sizeof(MigrationClass),
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,
> +    .instance_finalize = migration_instance_finalize,
>  };
>  
>  static void register_migration_types(void)
> -- 
> 2.14.0.rc0.1.g40ca67566
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Vladimir Sementsov-Ogievskiy]

Hi all!

Hmm, looks like leak is not fixed here: I've checked it while running 
iotest 181, that
migration_instance_finalize is not called.

If I understand correct, to call it we need unref current_migration 
object somewhere.

Or, may be I'm missing something..

01.08.2017 19:04, Marc-André Lureau wrote:
> Spotted thanks to valgrind and tests/device-introspect-test:
>
> ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> ==11711==    by 0x718328: object_new (object.c:493)
> ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   migration/migration.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 085c32c994..c3fe0ed9ca 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
>       dc->props = migration_properties;
>   }
>   
> +static void migration_instance_finalize(Object *obj)
> +{
> +    MigrationState *ms = MIGRATION_OBJ(obj);
> +    MigrationParameters *params = &ms->parameters;
> +
> +    g_free(params->tls_hostname);
> +    g_free(params->tls_creds);
> +}
> +
>   static void migration_instance_init(Object *obj)
>   {
>       MigrationState *ms = MIGRATION_OBJ(obj);
> @@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
>       .class_size = sizeof(MigrationClass),
>       .instance_size = sizeof(MigrationState),
>       .instance_init = migration_instance_init,
> +    .instance_finalize = migration_instance_finalize,
>   };
>   
>   static void register_migration_types(void)


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Peter Xu]

On Wed, Dec 27, 2017 at 03:25:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Hmm, looks like leak is not fixed here: I've checked it while running iotest
> 181, that
> migration_instance_finalize is not called.
> 
> If I understand correct, to call it we need unref current_migration object
> somewhere.
> 
> Or, may be I'm missing something..

I think you are right.

It does not matter much though since we don't dynamically allocate
migration object (there is only one and it lives forever).  Do you
want to post a patch?  I guess the safest place to unref it is at the
end of main() to make sure no one will be using it any more.

(Hmm, the incoming migration state is still static)

Thanks,

> 
> 01.08.2017 19:04, Marc-André Lureau wrote:
> > Spotted thanks to valgrind and tests/device-introspect-test:
> > 
> > ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> > ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> > ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> > ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> > ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> > ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> > ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> > ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> > ==11711==    by 0x718328: object_new (object.c:493)
> > ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> > ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> > ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> > ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   migration/migration.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 085c32c994..c3fe0ed9ca 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
> >       dc->props = migration_properties;
> >   }
> > +static void migration_instance_finalize(Object *obj)
> > +{
> > +    MigrationState *ms = MIGRATION_OBJ(obj);
> > +    MigrationParameters *params = &ms->parameters;
> > +
> > +    g_free(params->tls_hostname);
> > +    g_free(params->tls_creds);
> > +}
> > +
> >   static void migration_instance_init(Object *obj)
> >   {
> >       MigrationState *ms = MIGRATION_OBJ(obj);
> > @@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
> >       .class_size = sizeof(MigrationClass),
> >       .instance_size = sizeof(MigrationState),
> >       .instance_init = migration_instance_init,
> > +    .instance_finalize = migration_instance_finalize,
> >   };
> >   static void register_migration_types(void)
> 
> 
> -- 
> Best regards,
> Vladimir
> 
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Vladimir Sementsov-Ogievskiy]

28.12.2017 05:19, Peter Xu wrote:
> On Wed, Dec 27, 2017 at 03:25:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Hmm, looks like leak is not fixed here: I've checked it while running iotest
>> 181, that
>> migration_instance_finalize is not called.
>>
>> If I understand correct, to call it we need unref current_migration object
>> somewhere.
>>
>> Or, may be I'm missing something..
> I think you are right.
>
> It does not matter much though since we don't dynamically allocate
> migration object (there is only one and it lives forever).  Do you
> want to post a patch?  I guess the safest place to unref it is at the
> end of main() to make sure no one will be using it any more.
>
> (Hmm, the incoming migration state is still static)
>
> Thanks,

Ok, I'll send a patch.

>
>> 01.08.2017 19:04, Marc-André Lureau wrote:
>>> Spotted thanks to valgrind and tests/device-introspect-test:
>>>
>>> ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
>>> ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
>>> ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
>>> ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
>>> ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
>>> ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
>>> ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
>>> ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
>>> ==11711==    by 0x718328: object_new (object.c:493)
>>> ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
>>> ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
>>> ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
>>> ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    migration/migration.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 085c32c994..c3fe0ed9ca 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
>>>        dc->props = migration_properties;
>>>    }
>>> +static void migration_instance_finalize(Object *obj)
>>> +{
>>> +    MigrationState *ms = MIGRATION_OBJ(obj);
>>> +    MigrationParameters *params = &ms->parameters;
>>> +
>>> +    g_free(params->tls_hostname);
>>> +    g_free(params->tls_creds);
>>> +}
>>> +
>>>    static void migration_instance_init(Object *obj)
>>>    {
>>>        MigrationState *ms = MIGRATION_OBJ(obj);
>>> @@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
>>>        .class_size = sizeof(MigrationClass),
>>>        .instance_size = sizeof(MigrationState),
>>>        .instance_init = migration_instance_init,
>>> +    .instance_finalize = migration_instance_finalize,
>>>    };
>>>    static void register_migration_types(void)
>>
>> -- 
>> Best regards,
>> Vladimir
>>
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 28.12.2017 05:19, Peter Xu wrote:
> > On Wed, Dec 27, 2017 at 03:25:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Hi all!
> > > 
> > > Hmm, looks like leak is not fixed here: I've checked it while running iotest
> > > 181, that
> > > migration_instance_finalize is not called.
> > > 
> > > If I understand correct, to call it we need unref current_migration object
> > > somewhere.
> > > 
> > > Or, may be I'm missing something..
> > I think you are right.
> > 
> > It does not matter much though since we don't dynamically allocate
> > migration object (there is only one and it lives forever).  Do you
> > want to post a patch?  I guess the safest place to unref it is at the
> > end of main() to make sure no one will be using it any more.
> > 
> > (Hmm, the incoming migration state is still static)
> > 
> > Thanks,
> 
> Ok, I'll send a patch.

Be very very careful that it doesn't crash in cases like quit in the main thread
while a migration is still running.

I have no problem with this object living forever and letting it just
die with exit().

Dave


> > 
> > > 01.08.2017 19:04, Marc-André Lureau wrote:
> > > > Spotted thanks to valgrind and tests/device-introspect-test:
> > > > 
> > > > ==11711== 1 bytes in 1 blocks are definitely lost in loss record 6 of 14,537
> > > > ==11711==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> > > > ==11711==    by 0x1E0CDBD8: g_malloc (gmem.c:94)
> > > > ==11711==    by 0x1E0E696E: g_strdup (gstrfuncs.c:363)
> > > > ==11711==    by 0x695693: migration_instance_init (migration.c:2226)
> > > > ==11711==    by 0x717C4B: object_init_with_type (object.c:344)
> > > > ==11711==    by 0x717E80: object_initialize_with_type (object.c:375)
> > > > ==11711==    by 0x7182EB: object_new_with_type (object.c:483)
> > > > ==11711==    by 0x718328: object_new (object.c:493)
> > > > ==11711==    by 0x4B8A29: qmp_device_list_properties (qmp.c:542)
> > > > ==11711==    by 0x4A9561: qmp_marshal_device_list_properties (qmp-marshal.c:1425)
> > > > ==11711==    by 0x819D4A: do_qmp_dispatch (qmp-dispatch.c:104)
> > > > ==11711==    by 0x819E82: qmp_dispatch (qmp-dispatch.c:131)
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >    migration/migration.c | 10 ++++++++++
> > > >    1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index 085c32c994..c3fe0ed9ca 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -2214,6 +2214,15 @@ static void migration_class_init(ObjectClass *klass, void *data)
> > > >        dc->props = migration_properties;
> > > >    }
> > > > +static void migration_instance_finalize(Object *obj)
> > > > +{
> > > > +    MigrationState *ms = MIGRATION_OBJ(obj);
> > > > +    MigrationParameters *params = &ms->parameters;
> > > > +
> > > > +    g_free(params->tls_hostname);
> > > > +    g_free(params->tls_creds);
> > > > +}
> > > > +
> > > >    static void migration_instance_init(Object *obj)
> > > >    {
> > > >        MigrationState *ms = MIGRATION_OBJ(obj);
> > > > @@ -2282,6 +2291,7 @@ static const TypeInfo migration_type = {
> > > >        .class_size = sizeof(MigrationClass),
> > > >        .instance_size = sizeof(MigrationState),
> > > >        .instance_init = migration_instance_init,
> > > > +    .instance_finalize = migration_instance_finalize,
> > > >    };
> > > >    static void register_migration_types(void)
> > > 
> > > -- 
> > > Best regards,
> > > Vladimir
> > > 
> > > 
> 
> 
> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: fix small leaks

[Peter Xu]

On Tue, Jan 02, 2018 at 03:19:38PM +0000, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> > 28.12.2017 05:19, Peter Xu wrote:
> > > On Wed, Dec 27, 2017 at 03:25:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > Hi all!
> > > > 
> > > > Hmm, looks like leak is not fixed here: I've checked it while running iotest
> > > > 181, that
> > > > migration_instance_finalize is not called.
> > > > 
> > > > If I understand correct, to call it we need unref current_migration object
> > > > somewhere.
> > > > 
> > > > Or, may be I'm missing something..
> > > I think you are right.
> > > 
> > > It does not matter much though since we don't dynamically allocate
> > > migration object (there is only one and it lives forever).  Do you
> > > want to post a patch?  I guess the safest place to unref it is at the
> > > end of main() to make sure no one will be using it any more.
> > > 
> > > (Hmm, the incoming migration state is still static)
> > > 
> > > Thanks,
> > 
> > Ok, I'll send a patch.
> 
> Be very very careful that it doesn't crash in cases like quit in the main thread
> while a migration is still running.

Agree.

> 
> I have no problem with this object living forever and letting it just
> die with exit().

Yes.  But if better, I am thinking whether we should always make sure
migration is completed/failed/cancelled before that point.  If we quit
QEMU with a working migration stream, logically we should stop it
before leaving, as part of QEMU's cleanup path.

-- 
Peter Xu