Message ID | 20201215224133.3545901-2-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
Series | Fix test-char reference counting bug | expand |
On Wed, Dec 16, 2020 at 2:41 AM Eduardo Habkost <ehabkost@redhat.com> wrote: > commit 1e419ee68fa5 ("chardev: generate an internal id when none > given") changed the reference ownership semantics of > qemu_chardev_new(NULL, ...): now all chardevs created using > qemu_chardev_new() are added to the /chardevs QOM container, and > the caller does not own a reference to the newly created object. > > However, the code at char_file_test_internal() had not been > updated and was calling object_unref() on a chardev object it > didn't own. This makes the chardev be destroyed, but leaves a > dangling pointer in the /chardev container children list, and > seems to be the cause of the following char_serial_test() crash: > > Unexpected error in object_property_try_add() at ../qom/object.c:1220: \ > attempt to add duplicate property 'serial-id' to object (type > 'container') > ERROR test-char - too few tests run (expected 38, got 9) > > Update the code to use object_unparent() at the end of > char_file_test_internal(), to make sure the chardev will be > correctly removed from the QOM tree. > > Fixes: 1e419ee68fa5 ("chardev: generate an internal id when none given") > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > nice catch Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- > tests/test-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/test-char.c b/tests/test-char.c > index 953e0d1c1f..06102977b6 100644 > --- a/tests/test-char.c > +++ b/tests/test-char.c > @@ -1298,7 +1298,7 @@ static void char_file_test_internal(Chardev > *ext_chr, const char *filepath) > g_assert(strncmp(contents, "hello!", 6) == 0); > > if (!ext_chr) { > - object_unref(OBJECT(chr)); > + object_unparent(OBJECT(chr)); > g_unlink(out); > } > g_free(contents); > -- > 2.28.0 > >
Eduardo Habkost <ehabkost@redhat.com> writes: > commit 1e419ee68fa5 ("chardev: generate an internal id when none > given") changed the reference ownership semantics of > qemu_chardev_new(NULL, ...): now all chardevs created using > qemu_chardev_new() are added to the /chardevs QOM container, and > the caller does not own a reference to the newly created object. > > However, the code at char_file_test_internal() had not been > updated and was calling object_unref() on a chardev object it > didn't own. This makes the chardev be destroyed, but leaves a > dangling pointer in the /chardev container children list, and > seems to be the cause of the following char_serial_test() crash: > > Unexpected error in object_property_try_add() at ../qom/object.c:1220: \ > attempt to add duplicate property 'serial-id' to object (type 'container') > ERROR test-char - too few tests run (expected 38, got 9) > > Update the code to use object_unparent() at the end of > char_file_test_internal(), to make sure the chardev will be > correctly removed from the QOM tree. > > Fixes: 1e419ee68fa5 ("chardev: generate an internal id when none given") > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org> Previously I could get a failure within a 100 runs so I think the soak test is good ;-) Results summary: 0: 1000 times (100.00%), avg time 2.256 (0.00 varience/0.00 deviation) Ran command 1000 times, 1000 passes > --- > tests/test-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/test-char.c b/tests/test-char.c > index 953e0d1c1f..06102977b6 100644 > --- a/tests/test-char.c > +++ b/tests/test-char.c > @@ -1298,7 +1298,7 @@ static void char_file_test_internal(Chardev *ext_chr, const char *filepath) > g_assert(strncmp(contents, "hello!", 6) == 0); > > if (!ext_chr) { > - object_unref(OBJECT(chr)); > + object_unparent(OBJECT(chr)); > g_unlink(out); > } > g_free(contents);
diff --git a/tests/test-char.c b/tests/test-char.c index 953e0d1c1f..06102977b6 100644 --- a/tests/test-char.c +++ b/tests/test-char.c @@ -1298,7 +1298,7 @@ static void char_file_test_internal(Chardev *ext_chr, const char *filepath) g_assert(strncmp(contents, "hello!", 6) == 0); if (!ext_chr) { - object_unref(OBJECT(chr)); + object_unparent(OBJECT(chr)); g_unlink(out); } g_free(contents);
commit 1e419ee68fa5 ("chardev: generate an internal id when none given") changed the reference ownership semantics of qemu_chardev_new(NULL, ...): now all chardevs created using qemu_chardev_new() are added to the /chardevs QOM container, and the caller does not own a reference to the newly created object. However, the code at char_file_test_internal() had not been updated and was calling object_unref() on a chardev object it didn't own. This makes the chardev be destroyed, but leaves a dangling pointer in the /chardev container children list, and seems to be the cause of the following char_serial_test() crash: Unexpected error in object_property_try_add() at ../qom/object.c:1220: \ attempt to add duplicate property 'serial-id' to object (type 'container') ERROR test-char - too few tests run (expected 38, got 9) Update the code to use object_unparent() at the end of char_file_test_internal(), to make sure the chardev will be correctly removed from the QOM tree. Fixes: 1e419ee68fa5 ("chardev: generate an internal id when none given") Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- tests/test-char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)