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(-)

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

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);


-- 
Alex Bennée