| Message ID | 1576805214-2508-1-git-send-email-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] vhost-user-test: fix a memory leak | expand |
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.
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
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 > > . >
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 >> >> . >>
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
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
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 > > . >
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)