[PATCH V2] vhost-user-test: fix a memory leak

pannengyuan@huawei.com posted 1 patch 4 years, 3 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1576805214-2508-1-git-send-email-pannengyuan@huawei.com
Maintainers: Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
tests/vhost-user-test.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[PATCH V2] vhost-user-test: fix a memory leak
Posted by pannengyuan@huawei.com 4 years, 3 months ago
From: Pan Nengyuan <pannengyuan@huawei.com>

Spotted by ASAN.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Changes V2 to V1:
- use a "goto cleanup", instead of duplicating the "free" functions.
- free "dest_cmdline" at the end.
---
 tests/vhost-user-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 91ea373..dcb8617 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -717,7 +717,7 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
     guint64 size;
 
     if (!wait_for_fds(s)) {
-        return;
+        goto cleanup;
     }
 
     size = get_log_size(s);
@@ -776,8 +776,11 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
     g_source_unref(source);
 
     qtest_quit(to);
+
+ cleanup:
     test_server_free(dest);
     g_free(uri);
+    g_string_free(dest_cmdline, true);
 }
 
 static void wait_for_rings_started(TestServer *s, size_t count)
-- 
2.7.2.windows.1



Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Thomas Huth 4 years, 2 months ago
On 20/12/2019 02.26, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Spotted by ASAN.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes V2 to V1:
> - use a "goto cleanup", instead of duplicating the "free" functions.
> - free "dest_cmdline" at the end.
> ---
>  tests/vhost-user-test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 91ea373..dcb8617 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -717,7 +717,7 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>      guint64 size;
>  
>      if (!wait_for_fds(s)) {
> -        return;
> +        goto cleanup;
>      }
>  
>      size = get_log_size(s);
> @@ -776,8 +776,11 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>      g_source_unref(source);
>  
>      qtest_quit(to);
> +
> + cleanup:
>      test_server_free(dest);
>      g_free(uri);
> +    g_string_free(dest_cmdline, true);
>  }
>  
>  static void wait_for_rings_started(TestServer *s, size_t count)
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

... and picked up to my qtest-next tree.


Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Thomas Huth 4 years, 2 months ago
On 10/01/2020 15.07, Thomas Huth wrote:
> On 20/12/2019 02.26, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Spotted by ASAN.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Changes V2 to V1:
>> - use a "goto cleanup", instead of duplicating the "free" functions.
>> - free "dest_cmdline" at the end.
>> ---
>>  tests/vhost-user-test.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>> index 91ea373..dcb8617 100644
>> --- a/tests/vhost-user-test.c
>> +++ b/tests/vhost-user-test.c
>> @@ -717,7 +717,7 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>>      guint64 size;
>>  
>>      if (!wait_for_fds(s)) {
>> -        return;
>> +        goto cleanup;
>>      }
>>  
>>      size = get_log_size(s);
>> @@ -776,8 +776,11 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>>      g_source_unref(source);
>>  
>>      qtest_quit(to);
>> +
>> + cleanup:
>>      test_server_free(dest);
>>      g_free(uri);
>> +    g_string_free(dest_cmdline, true);
>>  }
>>  
>>  static void wait_for_rings_started(TestServer *s, size_t count)
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> ... and picked up to my qtest-next tree.

... and now I had to unqueue the patch again. It is reproducibly causing
one of the gitlab CI pipelines to fail with a timeout, e.g.:

 https://gitlab.com/huth/qemu/-/jobs/400101552

Not sure what is going on here, though, there is no obvious error
message in the output... this needs some more investigation... do you
have a gitlab account and could have a look?

 Thomas


Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Pan Nengyuan 4 years, 2 months ago

On 1/12/2020 6:39 PM, Thomas Huth wrote:
> On 10/01/2020 15.07, Thomas Huth wrote:
>> On 20/12/2019 02.26, pannengyuan@huawei.com wrote:
>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Spotted by ASAN.
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Changes V2 to V1:
>>> - use a "goto cleanup", instead of duplicating the "free" functions.
>>> - free "dest_cmdline" at the end.
>>> ---
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> ... and picked up to my qtest-next tree.
> 
> ... and now I had to unqueue the patch again. It is reproducibly causing
> one of the gitlab CI pipelines to fail with a timeout, e.g.:
> 
>  https://gitlab.com/huth/qemu/-/jobs/400101552
> 
> Not sure what is going on here, though, there is no obvious error
> message in the output... this needs some more investigation... do you
> have a gitlab account and could have a look?
> 

OK, I will register a account and have a look.

>  Thomas
> 
> .
> 

Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Pan Nengyuan 4 years, 2 months ago

On 1/13/2020 10:32 AM, Pan Nengyuan wrote:
> 
> 
> On 1/12/2020 6:39 PM, Thomas Huth wrote:
>> On 10/01/2020 15.07, Thomas Huth wrote:
>>> On 20/12/2019 02.26, pannengyuan@huawei.com wrote:
>>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>>
>>>> Spotted by ASAN.
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>>> ---
>>>> Changes V2 to V1:
>>>> - use a "goto cleanup", instead of duplicating the "free" functions.
>>>> - free "dest_cmdline" at the end.
>>>> ---
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> ... and picked up to my qtest-next tree.
>>
>> ... and now I had to unqueue the patch again. It is reproducibly causing
>> one of the gitlab CI pipelines to fail with a timeout, e.g.:
>>
>>  https://gitlab.com/huth/qemu/-/jobs/400101552
>>
>> Not sure what is going on here, though, there is no obvious error
>> message in the output... this needs some more investigation... do you
>> have a gitlab account and could have a look?
>>
> 
> OK, I will register a account and have a look.
> 

I'm sorry, I build and test with the same params, but I can't reproduce it.
Could you add "V=1 or V=2" params to get more information ?

>>  Thomas
>>
>> .
>>

Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Thomas Huth 4 years, 2 months ago
On 15/01/2020 04.10, Pan Nengyuan wrote:
> 
> On 1/13/2020 10:32 AM, Pan Nengyuan wrote:
>>
>> On 1/12/2020 6:39 PM, Thomas Huth wrote:
[...]
>>> ... and now I had to unqueue the patch again. It is reproducibly causing
>>> one of the gitlab CI pipelines to fail with a timeout, e.g.:
>>>
>>>  https://gitlab.com/huth/qemu/-/jobs/400101552
>>>
>>> Not sure what is going on here, though, there is no obvious error
>>> message in the output... this needs some more investigation... do you
>>> have a gitlab account and could have a look?
>>>
>>
>> OK, I will register a account and have a look.
>>
> 
> I'm sorry, I build and test with the same params, but I can't reproduce it.
> Could you add "V=1 or V=2" params to get more information ?

It seems to hang forever in qos-test
/arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/announce-self
:

 https://gitlab.com/huth/qemu/-/jobs/403472594

It's completely weird, I also added some fprintf statements:

 https://gitlab.com/huth/qemu/commit/8ae76c0cf37cf46d26620dd

... but none of them show up in the output of the test run... so I'm
currently completely puzzled what might be going wrong here... Any other
ideas what we could try here?

 Thomas


Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Thomas Huth 4 years, 2 months ago
On 15/01/2020 10.13, Thomas Huth wrote:
> On 15/01/2020 04.10, Pan Nengyuan wrote:
>>
>> On 1/13/2020 10:32 AM, Pan Nengyuan wrote:
>>>
>>> On 1/12/2020 6:39 PM, Thomas Huth wrote:
> [...]
>>>> ... and now I had to unqueue the patch again. It is reproducibly causing
>>>> one of the gitlab CI pipelines to fail with a timeout, e.g.:
>>>>
>>>>  https://gitlab.com/huth/qemu/-/jobs/400101552
>>>>
>>>> Not sure what is going on here, though, there is no obvious error
>>>> message in the output... this needs some more investigation... do you
>>>> have a gitlab account and could have a look?
>>>>
>>>
>>> OK, I will register a account and have a look.
>>>
>>
>> I'm sorry, I build and test with the same params, but I can't reproduce it.
>> Could you add "V=1 or V=2" params to get more information ?
> 
> It seems to hang forever in qos-test
> /arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/announce-self
> :
> 
>  https://gitlab.com/huth/qemu/-/jobs/403472594
> 
> It's completely weird, I also added some fprintf statements:
> 
>  https://gitlab.com/huth/qemu/commit/8ae76c0cf37cf46d26620dd
> 
> ... but none of them show up in the output of the test run... so I'm
> currently completely puzzled what might be going wrong here... Any other
> ideas what we could try here?

I tried to add some more fprintfs here and there to see where it hangs,
but I did not succeed to get any further.

However, the CI build succeeds with this fix instead:

diff a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -707,9 +707,9 @@ static void test_read_guest_mem(void *obj, void
*arg, QGuestAllocator *alloc)
 static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
 {
     TestServer *s = arg;
-    TestServer *dest = test_server_new("dest");
-    GString *dest_cmdline = g_string_new(qos_get_current_command_line());
-    char *uri = g_strdup_printf("%s%s", "unix:", dest->mig_path);
+    TestServer *dest;
+    GString *dest_cmdline;
+    char *uri;
     QTestState *to;
     GSource *source;
     QDict *rsp;
@@ -720,6 +720,10 @@ static void test_migrate(void *obj, void *arg,
QGuestAllocator *alloc)
         return;
     }

+    dest = test_server_new("dest");
+    dest_cmdline = g_string_new(qos_get_current_command_line());
+    uri = g_strdup_printf("%s%s", "unix:", dest->mig_path);
+
     size = get_log_size(s);
     g_assert_cmpint(size, ==, (256 * 1024 * 1024) / (VHOST_LOG_PAGE * 8));

@@ -778,6 +782,7 @@ static void test_migrate(void *obj, void *arg,
QGuestAllocator *alloc)
     qtest_quit(to);
     test_server_free(dest);
     g_free(uri);
+    g_string_free(dest_cmdline, true);
 }


Here's a build with that patch that succeeded:

 https://gitlab.com/huth/qemu/-/jobs/405357307

So if you agree with that patch, I think we should simply use that
version instead, ok?

 Thomas


Re: [PATCH V2] vhost-user-test: fix a memory leak
Posted by Pan Nengyuan 4 years, 2 months ago

On 1/16/2020 9:29 PM, Thomas Huth wrote:
> On 15/01/2020 10.13, Thomas Huth wrote:
>> On 15/01/2020 04.10, Pan Nengyuan wrote:
>>>
>>> On 1/13/2020 10:32 AM, Pan Nengyuan wrote:
>>>>
>>>> On 1/12/2020 6:39 PM, Thomas Huth wrote:
>> [...]
>>>>> ... and now I had to unqueue the patch again. It is reproducibly causing
>>>>> one of the gitlab CI pipelines to fail with a timeout, e.g.:
>>>>>
>>>>>  https://gitlab.com/huth/qemu/-/jobs/400101552
>>>>>
>>>>> Not sure what is going on here, though, there is no obvious error
>>>>> message in the output... this needs some more investigation... do you
>>>>> have a gitlab account and could have a look?
>>>>>
>>>>
>>>> OK, I will register a account and have a look.
>>>>
>>>
>>> I'm sorry, I build and test with the same params, but I can't reproduce it.
>>> Could you add "V=1 or V=2" params to get more information ?
>>
>> It seems to hang forever in qos-test
>> /arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/announce-self
>> :
>>
>>  https://gitlab.com/huth/qemu/-/jobs/403472594
>>
>> It's completely weird, I also added some fprintf statements:
>>
>>  https://gitlab.com/huth/qemu/commit/8ae76c0cf37cf46d26620dd
>>
>> ... but none of them show up in the output of the test run... so I'm
>> currently completely puzzled what might be going wrong here... Any other
>> ideas what we could try here?
> 
> I tried to add some more fprintfs here and there to see where it hangs,
> but I did not succeed to get any further.
> 
> However, the CI build succeeds with this fix instead:
> 
> diff a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -707,9 +707,9 @@ static void test_read_guest_mem(void *obj, void
> *arg, QGuestAllocator *alloc)
>  static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>  {
>      TestServer *s = arg;
> -    TestServer *dest = test_server_new("dest");
> -    GString *dest_cmdline = g_string_new(qos_get_current_command_line());
> -    char *uri = g_strdup_printf("%s%s", "unix:", dest->mig_path);
> +    TestServer *dest;
> +    GString *dest_cmdline;
> +    char *uri;
>      QTestState *to;
>      GSource *source;
>      QDict *rsp;
> @@ -720,6 +720,10 @@ static void test_migrate(void *obj, void *arg,
> QGuestAllocator *alloc)
>          return;
>      }
> 
> +    dest = test_server_new("dest");
> +    dest_cmdline = g_string_new(qos_get_current_command_line());
> +    uri = g_strdup_printf("%s%s", "unix:", dest->mig_path);
> +
>      size = get_log_size(s);
>      g_assert_cmpint(size, ==, (256 * 1024 * 1024) / (VHOST_LOG_PAGE * 8));
> 
> @@ -778,6 +782,7 @@ static void test_migrate(void *obj, void *arg,
> QGuestAllocator *alloc)
>      qtest_quit(to);
>      test_server_free(dest);
>      g_free(uri);
> +    g_string_free(dest_cmdline, true);
>  }
> 
> 
> Here's a build with that patch that succeeded:
> 
>  https://gitlab.com/huth/qemu/-/jobs/405357307
> 
> So if you agree with that patch, I think we should simply use that
> version instead, ok?

Ok, I agree with it.

Thanks.

> 
>  Thomas
> 
> .
>