QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2] vhost-user-test: fix a memory leak
@ 2019-12-20  1:26 pannengyuan
  2020-01-10 14:07 ` Thomas Huth
  0 siblings, 1 reply; 8+ messages in thread
From: pannengyuan @ 2019-12-20  1:26 UTC (permalink / raw)
  To: thuth, lvivier, pbonzini; +Cc: Pan Nengyuan, zhang.zhanghailiang, qemu-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2019-12-20  1:26 [PATCH V2] vhost-user-test: fix a memory leak pannengyuan
@ 2020-01-10 14:07 ` Thomas Huth
  2020-01-12 10:39   ` Thomas Huth
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2020-01-10 14:07 UTC (permalink / raw)
  To: pannengyuan, lvivier, pbonzini; +Cc: zhang.zhanghailiang, qemu-devel

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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-10 14:07 ` Thomas Huth
@ 2020-01-12 10:39   ` Thomas Huth
  2020-01-13  2:32     ` Pan Nengyuan
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2020-01-12 10:39 UTC (permalink / raw)
  To: pannengyuan, lvivier, pbonzini; +Cc: zhang.zhanghailiang, qemu-devel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-12 10:39   ` Thomas Huth
@ 2020-01-13  2:32     ` Pan Nengyuan
  2020-01-15  3:10       ` Pan Nengyuan
  0 siblings, 1 reply; 8+ messages in thread
From: Pan Nengyuan @ 2020-01-13  2:32 UTC (permalink / raw)
  To: Thomas Huth, lvivier, pbonzini; +Cc: zhang.zhanghailiang, qemu-devel



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-13  2:32     ` Pan Nengyuan
@ 2020-01-15  3:10       ` Pan Nengyuan
  2020-01-15  9:13         ` Thomas Huth
  0 siblings, 1 reply; 8+ messages in thread
From: Pan Nengyuan @ 2020-01-15  3:10 UTC (permalink / raw)
  To: Thomas Huth; +Cc: lvivier, pbonzini, zhang.zhanghailiang, qemu-devel



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-15  3:10       ` Pan Nengyuan
@ 2020-01-15  9:13         ` Thomas Huth
  2020-01-16 13:29           ` Thomas Huth
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2020-01-15  9:13 UTC (permalink / raw)
  To: Pan Nengyuan; +Cc: lvivier, pbonzini, zhang.zhanghailiang, qemu-devel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-15  9:13         ` Thomas Huth
@ 2020-01-16 13:29           ` Thomas Huth
  2020-01-17  0:44             ` Pan Nengyuan
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2020-01-16 13:29 UTC (permalink / raw)
  To: Pan Nengyuan; +Cc: lvivier, pbonzini, zhang.zhanghailiang, qemu-devel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] vhost-user-test: fix a memory leak
  2020-01-16 13:29           ` Thomas Huth
@ 2020-01-17  0:44             ` Pan Nengyuan
  0 siblings, 0 replies; 8+ messages in thread
From: Pan Nengyuan @ 2020-01-17  0:44 UTC (permalink / raw)
  To: Thomas Huth; +Cc: lvivier, pbonzini, zhang.zhanghailiang, qemu-devel



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20  1:26 [PATCH V2] vhost-user-test: fix a memory leak pannengyuan
2020-01-10 14:07 ` Thomas Huth
2020-01-12 10:39   ` Thomas Huth
2020-01-13  2:32     ` Pan Nengyuan
2020-01-15  3:10       ` Pan Nengyuan
2020-01-15  9:13         ` Thomas Huth
2020-01-16 13:29           ` Thomas Huth
2020-01-17  0:44             ` Pan Nengyuan

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git