[PATCH v3 0/4] yank: Add chardev tests and fixes

[PATCH v3 0/4] yank: Add chardev tests and fixes

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

Hello Everyone,
These patches increase test coverage for yank, add tests and fix bugs and
crashes in yank in combination with chardev-change.
Please Review.

Regards,
Lukas Straub

Changes:
-v3:
 -Base on <cover.1616521341.git.lukasstraub2@web.de>
  ([PATCH 0/2] yank: Always link full yank code)
 -Drop patch 1 (tests: Use the normal yank code instead of stubs in relevant tests)

-v2:
 -test: add license
 -test: factorize testcases to a single function
 -test: test chardev_change with initialization of new chardev failing
 -fix chardev_change with initialization of new chardev failing
 -add reviewed-by and tested-by tags

Based-on: <cover.1616521341.git.lukasstraub2@web.de>
([PATCH 0/2] yank: Always link full yank code)

Lukas Straub (4):
  tests: Add tests for yank with the chardev-change case
  chardev/char.c: Move object_property_try_add_child out of chardev_new
  chardev/char.c: Always pass id to chardev_new
  chardev: Fix yank with the chardev-change case

 MAINTAINERS            |   1 +
 chardev/char-socket.c  |  20 +++-
 chardev/char.c         |  77 +++++++++++-----
 include/chardev/char.h |   3 +
 tests/unit/meson.build |   3 +-
 tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 276 insertions(+), 29 deletions(-)
 create mode 100644 tests/unit/test-yank.c

--
2.30.2

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/4] tests: Add tests for yank with the chardev-change case

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 8066 bytes --]

Add tests for yank with the chardev-change case.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 MAINTAINERS            |   1 +
 tests/unit/meson.build |   3 +-
 tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test-yank.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 77259c031d..accb683a55 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
 S: Odd fixes
 F: util/yank.c
 F: migration/yank_functions*
+F: tests/unit/test-yank.c
 F: include/qemu/yank.h
 F: qapi/yank.json

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 4bfe4627ba..b3bc2109da 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -123,7 +123,8 @@ if have_system
     'test-util-sockets': ['socket-helpers.c'],
     'test-base64': [],
     'test-bufferiszero': [],
-    'test-vmstate': [migration, io]
+    'test-vmstate': [migration, io],
+    'test-yank': ['socket-helpers.c', qom, io, chardev]
   }
   if 'CONFIG_INOTIFY1' in config_host
     tests += {'test-util-filemonitor': []}
diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
new file mode 100644
index 0000000000..5cb94c2fe4
--- /dev/null
+++ b/tests/unit/test-yank.c
@@ -0,0 +1,201 @@
+/*
+ * Tests for QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub <lukasstraub2@web.de>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <glib/gstdio.h>
+
+#include "qemu/config-file.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-char.h"
+#include "qapi/qapi-types-char.h"
+#include "qapi/qapi-commands-yank.h"
+#include "qapi/qapi-types-yank.h"
+#include "io/channel-socket.h"
+#include "socket-helpers.h"
+
+typedef struct {
+    SocketAddress *addr;
+    bool old_yank;
+    bool new_yank;
+    bool fail;
+} CharChangeTestConfig;
+
+static int chardev_change(void *opaque)
+{
+    return 0;
+}
+
+static bool is_yank_instance_registered(void)
+{
+    YankInstanceList *list;
+    bool ret;
+
+    list = qmp_query_yank(&error_abort);
+
+    ret = !!list;
+
+    qapi_free_YankInstanceList(list);
+
+    return ret;
+}
+
+static void char_change_test(gconstpointer opaque)
+{
+    CharChangeTestConfig *conf = (gpointer) opaque;
+    SocketAddress *addr;
+    Chardev *chr;
+    CharBackend be;
+    ChardevReturn *ret;
+    QIOChannelSocket *ioc;
+
+    /*
+     * Setup a listener socket and determine its address
+     * so we know the TCP port for the client later
+     */
+    ioc = qio_channel_socket_new();
+    g_assert_nonnull(ioc);
+    qio_channel_socket_listen_sync(ioc, conf->addr, 1, &error_abort);
+    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+    g_assert_nonnull(addr);
+
+    ChardevBackend backend[2] = {
+        /* doesn't support yank */
+        { .type = CHARDEV_BACKEND_KIND_NULL },
+        /* supports yank */
+        {
+            .type = CHARDEV_BACKEND_KIND_SOCKET,
+            .u.socket.data = &(ChardevSocket) {
+                .addr = &(SocketAddressLegacy) {
+                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+                    .u.inet.data = &addr->u.inet
+                },
+                .has_server = true,
+                .server = false
+            }
+        } };
+
+    ChardevBackend fail_backend[2] = {
+        /* doesn't support yank */
+        {
+            .type = CHARDEV_BACKEND_KIND_UDP,
+            .u.udp.data = &(ChardevUdp) {
+                .remote = &(SocketAddressLegacy) {
+                    .type = SOCKET_ADDRESS_LEGACY_KIND_UNIX,
+                    .u.q_unix.data = &(UnixSocketAddress) {
+                        .path = (char *)""
+                    }
+                }
+            }
+        },
+        /* supports yank */
+        {
+            .type = CHARDEV_BACKEND_KIND_SOCKET,
+            .u.socket.data = &(ChardevSocket) {
+                .addr = &(SocketAddressLegacy) {
+                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+                    .u.inet.data = &(InetSocketAddress) {
+                        .host = (char *)"127.0.0.1",
+                        .port = (char *)"0"
+                    }
+                },
+                .has_server = true,
+                .server = false
+            }
+        } };
+
+    g_assert(!is_yank_instance_registered());
+
+    ret = qmp_chardev_add("chardev", &backend[conf->old_yank], &error_abort);
+    qapi_free_ChardevReturn(ret);
+    chr = qemu_chr_find("chardev");
+    g_assert_nonnull(chr);
+
+    g_assert(is_yank_instance_registered() == conf->old_yank);
+
+    qemu_chr_wait_connected(chr, &error_abort);
+    qemu_chr_fe_init(&be, chr, &error_abort);
+    /* allow chardev-change */
+    qemu_chr_fe_set_handlers(&be, NULL, NULL,
+                             NULL, chardev_change, NULL, NULL, true);
+
+    if (conf->fail) {
+        g_setenv("QTEST_SILENT_ERRORS", "1", 1);
+        ret = qmp_chardev_change("chardev", &fail_backend[conf->new_yank],
+                                 NULL);
+        g_assert_null(ret);
+        g_assert(be.chr == chr);
+        g_assert(is_yank_instance_registered() == conf->old_yank);
+        g_unsetenv("QTEST_SILENT_ERRORS");
+    } else {
+        ret = qmp_chardev_change("chardev", &backend[conf->new_yank],
+                                 &error_abort);
+        g_assert_nonnull(ret);
+        g_assert(be.chr != chr);
+        g_assert(is_yank_instance_registered() == conf->new_yank);
+    }
+
+    object_unparent(OBJECT(be.chr));
+    object_unref(OBJECT(ioc));
+    qapi_free_ChardevReturn(ret);
+}
+
+static SocketAddress tcpaddr = {
+    .type = SOCKET_ADDRESS_TYPE_INET,
+    .u.inet.host = (char *)"127.0.0.1",
+    .u.inet.port = (char *)"0",
+};
+
+int main(int argc, char **argv)
+{
+    bool has_ipv4, has_ipv6;
+
+    qemu_init_main_loop(&error_abort);
+    socket_init();
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (socket_check_protocol_support(&has_ipv4, &has_ipv6) < 0) {
+        g_printerr("socket_check_protocol_support() failed\n");
+        goto end;
+    }
+
+    if (!has_ipv4) {
+        goto end;
+    }
+
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_chardev_opts);
+
+#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank)                           \
+    do {                                                                       \
+        g_test_add_data_func("/yank/char_change/success/" # name,              \
+                             &(CharChangeTestConfig) { .addr = &tcpaddr,       \
+                                                       .old_yank = (_old_yank),\
+                                                       .new_yank = (_new_yank),\
+                                                       .fail = false },        \
+                             char_change_test);                                \
+        g_test_add_data_func("/yank/char_change/fail/" # name,                 \
+                             &(CharChangeTestConfig) { .addr = &tcpaddr,       \
+                                                       .old_yank = (_old_yank),\
+                                                       .new_yank = (_new_yank),\
+                                                       .fail = true },         \
+                             char_change_test);                                \
+    } while (0)
+
+    CHAR_CHANGE_TEST(to_yank, false, true);
+    CHAR_CHANGE_TEST(yank_to_yank, true, true);
+    CHAR_CHANGE_TEST(from_yank, true, false);
+
+end:
+    return g_test_run();
+}
--
2.30.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/4] tests: Add tests for yank with the chardev-change case

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 8920 bytes --]

Hi

On Tue, Mar 23, 2021 at 9:57 PM Lukas Straub <lukasstraub2@web.de> wrote:

> Add tests for yank with the chardev-change case.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  MAINTAINERS            |   1 +
>  tests/unit/meson.build |   3 +-
>  tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 1 deletion(-)
>  create mode 100644 tests/unit/test-yank.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77259c031d..accb683a55 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
>  S: Odd fixes
>  F: util/yank.c
>  F: migration/yank_functions*
> +F: tests/unit/test-yank.c
>  F: include/qemu/yank.h
>  F: qapi/yank.json
>
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 4bfe4627ba..b3bc2109da 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -123,7 +123,8 @@ if have_system
>      'test-util-sockets': ['socket-helpers.c'],
>      'test-base64': [],
>      'test-bufferiszero': [],
> -    'test-vmstate': [migration, io]
> +    'test-vmstate': [migration, io],
> +    'test-yank': ['socket-helpers.c', qom, io, chardev]
>    }
>    if 'CONFIG_INOTIFY1' in config_host
>      tests += {'test-util-filemonitor': []}
> diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
> new file mode 100644
> index 0000000000..5cb94c2fe4
> --- /dev/null
> +++ b/tests/unit/test-yank.c
> @@ -0,0 +1,201 @@
> +/*
> + * Tests for QEMU yank feature
> + *
> + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +
> +#include "qemu/config-file.h"
> +#include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "chardev/char-fe.h"
> +#include "sysemu/sysemu.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-char.h"
> +#include "qapi/qapi-types-char.h"
> +#include "qapi/qapi-commands-yank.h"
> +#include "qapi/qapi-types-yank.h"
> +#include "io/channel-socket.h"
> +#include "socket-helpers.h"
> +
> +typedef struct {
> +    SocketAddress *addr;
> +    bool old_yank;
> +    bool new_yank;
> +    bool fail;
> +} CharChangeTestConfig;
> +
> +static int chardev_change(void *opaque)
> +{
> +    return 0;
> +}
> +
> +static bool is_yank_instance_registered(void)
> +{
> +    YankInstanceList *list;
> +    bool ret;
> +
> +    list = qmp_query_yank(&error_abort);
> +
> +    ret = !!list;
> +
> +    qapi_free_YankInstanceList(list);
> +
> +    return ret;
> +}
> +
> +static void char_change_test(gconstpointer opaque)
> +{
> +    CharChangeTestConfig *conf = (gpointer) opaque;
> +    SocketAddress *addr;
> +    Chardev *chr;
> +    CharBackend be;
> +    ChardevReturn *ret;
> +    QIOChannelSocket *ioc;
> +
> +    /*
> +     * Setup a listener socket and determine its address
> +     * so we know the TCP port for the client later
> +     */
> +    ioc = qio_channel_socket_new();
> +    g_assert_nonnull(ioc);
> +    qio_channel_socket_listen_sync(ioc, conf->addr, 1, &error_abort);
> +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> +    g_assert_nonnull(addr);
> +
> +    ChardevBackend backend[2] = {
> +        /* doesn't support yank */
> +        { .type = CHARDEV_BACKEND_KIND_NULL },
> +        /* supports yank */
> +        {
> +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> +            .u.socket.data = &(ChardevSocket) {
> +                .addr = &(SocketAddressLegacy) {
> +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> +                    .u.inet.data = &addr->u.inet
> +                },
> +                .has_server = true,
> +                .server = false
> +            }
> +        } };
> +
> +    ChardevBackend fail_backend[2] = {
> +        /* doesn't support yank */
> +        {
> +            .type = CHARDEV_BACKEND_KIND_UDP,
> +            .u.udp.data = &(ChardevUdp) {
> +                .remote = &(SocketAddressLegacy) {
> +                    .type = SOCKET_ADDRESS_LEGACY_KIND_UNIX,
> +                    .u.q_unix.data = &(UnixSocketAddress) {
> +                        .path = (char *)""
> +                    }
> +                }
> +            }
> +        },
> +        /* supports yank */
> +        {
> +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> +            .u.socket.data = &(ChardevSocket) {
> +                .addr = &(SocketAddressLegacy) {
> +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> +                    .u.inet.data = &(InetSocketAddress) {
> +                        .host = (char *)"127.0.0.1",
> +                        .port = (char *)"0"
> +                    }
> +                },
> +                .has_server = true,
> +                .server = false
> +            }
> +        } };
> +
> +    g_assert(!is_yank_instance_registered());
> +
> +    ret = qmp_chardev_add("chardev", &backend[conf->old_yank],
> &error_abort);
> +    qapi_free_ChardevReturn(ret);
> +    chr = qemu_chr_find("chardev");
> +    g_assert_nonnull(chr);
> +
> +    g_assert(is_yank_instance_registered() == conf->old_yank);
> +
> +    qemu_chr_wait_connected(chr, &error_abort);
> +    qemu_chr_fe_init(&be, chr, &error_abort);
> +    /* allow chardev-change */
> +    qemu_chr_fe_set_handlers(&be, NULL, NULL,
> +                             NULL, chardev_change, NULL, NULL, true);
> +
> +    if (conf->fail) {
> +        g_setenv("QTEST_SILENT_ERRORS", "1", 1);
> +        ret = qmp_chardev_change("chardev", &fail_backend[conf->new_yank],
> +                                 NULL);
> +        g_assert_null(ret);
> +        g_assert(be.chr == chr);
> +        g_assert(is_yank_instance_registered() == conf->old_yank);
> +        g_unsetenv("QTEST_SILENT_ERRORS");
> +    } else {
> +        ret = qmp_chardev_change("chardev", &backend[conf->new_yank],
> +                                 &error_abort);
> +        g_assert_nonnull(ret);
> +        g_assert(be.chr != chr);
> +        g_assert(is_yank_instance_registered() == conf->new_yank);
> +    }
> +
> +    object_unparent(OBJECT(be.chr));
> +    object_unref(OBJECT(ioc));
> +    qapi_free_ChardevReturn(ret);
> +}
> +
> +static SocketAddress tcpaddr = {
> +    .type = SOCKET_ADDRESS_TYPE_INET,
> +    .u.inet.host = (char *)"127.0.0.1",
> +    .u.inet.port = (char *)"0",
> +};
> +
> +int main(int argc, char **argv)
> +{
> +    bool has_ipv4, has_ipv6;
> +
> +    qemu_init_main_loop(&error_abort);
> +    socket_init();
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (socket_check_protocol_support(&has_ipv4, &has_ipv6) < 0) {
> +        g_printerr("socket_check_protocol_support() failed\n");
> +        goto end;
> +    }
> +
> +    if (!has_ipv4) {
> +        goto end;
> +    }
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    qemu_add_opts(&qemu_chardev_opts);
> +
> +#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank)
>      \
> +    do {
>      \
> +        g_test_add_data_func("/yank/char_change/success/" # name,
>       \
> +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
>      \
> +                                                       .old_yank =
> (_old_yank),\
> +                                                       .new_yank =
> (_new_yank),\
> +                                                       .fail = false },
>       \
> +                             char_change_test);
>       \
> +        g_test_add_data_func("/yank/char_change/fail/" # name,
>      \
> +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
>      \
> +                                                       .old_yank =
> (_old_yank),\
> +                                                       .new_yank =
> (_new_yank),\
> +                                                       .fail = true },
>      \
> +                             char_change_test);
>       \
> +    } while (0)
> +
>

If you run the test under ASAN, you get an error. Config is
stack-use-after-scope.

Otherwise, the test looks good to me, but please introduce the test after
the fix in the series, to avoid intermittent breaking when doing git
rebase. Iow, we make our best to have no regressions introduced in a final
series, even temporarily in tests.

thanks



> +    CHAR_CHANGE_TEST(to_yank, false, true);
> +    CHAR_CHANGE_TEST(yank_to_yank, true, true);
> +    CHAR_CHANGE_TEST(from_yank, true, false);
> +
> +end:
> +    return g_test_run();
> +}
> --
> 2.30.2
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 11745 bytes --]

Re: [PATCH v3 1/4] tests: Add tests for yank with the chardev-change case

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 9703 bytes --]

On Fri, 26 Mar 2021 00:36:45 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Mar 23, 2021 at 9:57 PM Lukas Straub <lukasstraub2@web.de> wrote:
> 
> > Add tests for yank with the chardev-change case.
> >
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  MAINTAINERS            |   1 +
> >  tests/unit/meson.build |   3 +-
> >  tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/unit/test-yank.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 77259c031d..accb683a55 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
> >  S: Odd fixes
> >  F: util/yank.c
> >  F: migration/yank_functions*
> > +F: tests/unit/test-yank.c
> >  F: include/qemu/yank.h
> >  F: qapi/yank.json
> >
> > diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> > index 4bfe4627ba..b3bc2109da 100644
> > --- a/tests/unit/meson.build
> > +++ b/tests/unit/meson.build
> > @@ -123,7 +123,8 @@ if have_system
> >      'test-util-sockets': ['socket-helpers.c'],
> >      'test-base64': [],
> >      'test-bufferiszero': [],
> > -    'test-vmstate': [migration, io]
> > +    'test-vmstate': [migration, io],
> > +    'test-yank': ['socket-helpers.c', qom, io, chardev]
> >    }
> >    if 'CONFIG_INOTIFY1' in config_host
> >      tests += {'test-util-filemonitor': []}
> > diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
> > new file mode 100644
> > index 0000000000..5cb94c2fe4
> > --- /dev/null
> > +++ b/tests/unit/test-yank.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * Tests for QEMU yank feature
> > + *
> > + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include <glib/gstdio.h>
> > +
> > +#include "qemu/config-file.h"
> > +#include "qemu/module.h"
> > +#include "qemu/option.h"
> > +#include "chardev/char-fe.h"
> > +#include "sysemu/sysemu.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qapi-commands-char.h"
> > +#include "qapi/qapi-types-char.h"
> > +#include "qapi/qapi-commands-yank.h"
> > +#include "qapi/qapi-types-yank.h"
> > +#include "io/channel-socket.h"
> > +#include "socket-helpers.h"
> > +
> > +typedef struct {
> > +    SocketAddress *addr;
> > +    bool old_yank;
> > +    bool new_yank;
> > +    bool fail;
> > +} CharChangeTestConfig;
> > +
> > +static int chardev_change(void *opaque)
> > +{
> > +    return 0;
> > +}
> > +
> > +static bool is_yank_instance_registered(void)
> > +{
> > +    YankInstanceList *list;
> > +    bool ret;
> > +
> > +    list = qmp_query_yank(&error_abort);
> > +
> > +    ret = !!list;
> > +
> > +    qapi_free_YankInstanceList(list);
> > +
> > +    return ret;
> > +}
> > +
> > +static void char_change_test(gconstpointer opaque)
> > +{
> > +    CharChangeTestConfig *conf = (gpointer) opaque;
> > +    SocketAddress *addr;
> > +    Chardev *chr;
> > +    CharBackend be;
> > +    ChardevReturn *ret;
> > +    QIOChannelSocket *ioc;
> > +
> > +    /*
> > +     * Setup a listener socket and determine its address
> > +     * so we know the TCP port for the client later
> > +     */
> > +    ioc = qio_channel_socket_new();
> > +    g_assert_nonnull(ioc);
> > +    qio_channel_socket_listen_sync(ioc, conf->addr, 1, &error_abort);
> > +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> > +    g_assert_nonnull(addr);
> > +
> > +    ChardevBackend backend[2] = {
> > +        /* doesn't support yank */
> > +        { .type = CHARDEV_BACKEND_KIND_NULL },
> > +        /* supports yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> > +            .u.socket.data = &(ChardevSocket) {
> > +                .addr = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> > +                    .u.inet.data = &addr->u.inet
> > +                },
> > +                .has_server = true,
> > +                .server = false
> > +            }
> > +        } };
> > +
> > +    ChardevBackend fail_backend[2] = {
> > +        /* doesn't support yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_UDP,
> > +            .u.udp.data = &(ChardevUdp) {
> > +                .remote = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_UNIX,
> > +                    .u.q_unix.data = &(UnixSocketAddress) {
> > +                        .path = (char *)""
> > +                    }
> > +                }
> > +            }
> > +        },
> > +        /* supports yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> > +            .u.socket.data = &(ChardevSocket) {
> > +                .addr = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> > +                    .u.inet.data = &(InetSocketAddress) {
> > +                        .host = (char *)"127.0.0.1",
> > +                        .port = (char *)"0"
> > +                    }
> > +                },
> > +                .has_server = true,
> > +                .server = false
> > +            }
> > +        } };
> > +
> > +    g_assert(!is_yank_instance_registered());
> > +
> > +    ret = qmp_chardev_add("chardev", &backend[conf->old_yank],
> > &error_abort);
> > +    qapi_free_ChardevReturn(ret);
> > +    chr = qemu_chr_find("chardev");
> > +    g_assert_nonnull(chr);
> > +
> > +    g_assert(is_yank_instance_registered() == conf->old_yank);
> > +
> > +    qemu_chr_wait_connected(chr, &error_abort);
> > +    qemu_chr_fe_init(&be, chr, &error_abort);
> > +    /* allow chardev-change */
> > +    qemu_chr_fe_set_handlers(&be, NULL, NULL,
> > +                             NULL, chardev_change, NULL, NULL, true);
> > +
> > +    if (conf->fail) {
> > +        g_setenv("QTEST_SILENT_ERRORS", "1", 1);
> > +        ret = qmp_chardev_change("chardev", &fail_backend[conf->new_yank],
> > +                                 NULL);
> > +        g_assert_null(ret);
> > +        g_assert(be.chr == chr);
> > +        g_assert(is_yank_instance_registered() == conf->old_yank);
> > +        g_unsetenv("QTEST_SILENT_ERRORS");
> > +    } else {
> > +        ret = qmp_chardev_change("chardev", &backend[conf->new_yank],
> > +                                 &error_abort);
> > +        g_assert_nonnull(ret);
> > +        g_assert(be.chr != chr);
> > +        g_assert(is_yank_instance_registered() == conf->new_yank);
> > +    }
> > +
> > +    object_unparent(OBJECT(be.chr));
> > +    object_unref(OBJECT(ioc));
> > +    qapi_free_ChardevReturn(ret);
> > +}
> > +
> > +static SocketAddress tcpaddr = {
> > +    .type = SOCKET_ADDRESS_TYPE_INET,
> > +    .u.inet.host = (char *)"127.0.0.1",
> > +    .u.inet.port = (char *)"0",
> > +};
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    bool has_ipv4, has_ipv6;
> > +
> > +    qemu_init_main_loop(&error_abort);
> > +    socket_init();
> > +
> > +    g_test_init(&argc, &argv, NULL);
> > +
> > +    if (socket_check_protocol_support(&has_ipv4, &has_ipv6) < 0) {
> > +        g_printerr("socket_check_protocol_support() failed\n");
> > +        goto end;
> > +    }
> > +
> > +    if (!has_ipv4) {
> > +        goto end;
> > +    }
> > +
> > +    module_call_init(MODULE_INIT_QOM);
> > +    qemu_add_opts(&qemu_chardev_opts);
> > +
> > +#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank)
> >      \
> > +    do {
> >      \
> > +        g_test_add_data_func("/yank/char_change/success/" # name,
> >       \
> > +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
> >      \
> > +                                                       .old_yank =
> > (_old_yank),\
> > +                                                       .new_yank =
> > (_new_yank),\
> > +                                                       .fail = false },
> >       \
> > +                             char_change_test);
> >       \
> > +        g_test_add_data_func("/yank/char_change/fail/" # name,
> >      \
> > +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
> >      \
> > +                                                       .old_yank =
> > (_old_yank),\
> > +                                                       .new_yank =
> > (_new_yank),\
> > +                                                       .fail = true },
> >      \
> > +                             char_change_test);
> >       \
> > +    } while (0)
> > +
> >
> 
> If you run the test under ASAN, you get an error. Config is
> stack-use-after-scope.

Dammit, the do { } while(0) was a last-minute addition because checkpatch.pl
complained about it. I'll fix it.

> Otherwise, the test looks good to me, but please introduce the test after
> the fix in the series, to avoid intermittent breaking when doing git
> rebase. Iow, we make our best to have no regressions introduced in a final
> series, even temporarily in tests.

Regards,
Lukas Straub

> thanks
> 
> 
> 
> > +    CHAR_CHANGE_TEST(to_yank, false, true);
> > +    CHAR_CHANGE_TEST(yank_to_yank, true, true);
> > +    CHAR_CHANGE_TEST(from_yank, true, false);
> > +
> > +end:
> > +    return g_test_run();
> > +}
> > --
> > 2.30.2
> >
> >
> 



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/4] chardev/char.c: Move object_property_try_add_child out of chardev_new

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

Move object_property_try_add_child out of chardev_new into it's
callers. This is a preparation for the next patches to fix yank
with the chardev-change case.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Li Zhang <li.zhang@cloud.ionos.com>
---
 chardev/char.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 140d6d9d36..48f321b3e1 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -975,7 +975,9 @@ static Chardev *chardev_new(const char *id, const char *typename,

     qemu_char_open(chr, backend, &be_opened, &local_err);
     if (local_err) {
-        goto end;
+        error_propagate(errp, local_err);
+        object_unref(obj);
+        return NULL;
     }

     if (!chr->filename) {
@@ -985,22 +987,6 @@ static Chardev *chardev_new(const char *id, const char *typename,
         qemu_chr_be_event(chr, CHR_EVENT_OPENED);
     }

-    if (id) {
-        object_property_try_add_child(get_chardevs_root(), id, obj,
-                                      &local_err);
-        if (local_err) {
-            goto end;
-        }
-        object_unref(obj);
-    }
-
-end:
-    if (local_err) {
-        error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
-    }
-
     return chr;
 }

@@ -1009,6 +995,7 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
                           GMainContext *gcontext,
                           Error **errp)
 {
+    Chardev *chr;
     g_autofree char *genid = NULL;

     if (!id) {
@@ -1016,7 +1003,19 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
         id = genid;
     }

-    return chardev_new(id, typename, backend, gcontext, errp);
+    chr = chardev_new(id, typename, backend, gcontext, errp);
+    if (!chr) {
+        return NULL;
+    }
+
+    if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+                                       errp)) {
+        object_unref(OBJECT(chr));
+        return NULL;
+    }
+    object_unref(OBJECT(chr));
+
+    return chr;
 }

 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
@@ -1037,6 +1036,13 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         return NULL;
     }

+    if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+                                       errp)) {
+        object_unref(OBJECT(chr));
+        return NULL;
+    }
+    object_unref(OBJECT(chr));
+
     ret = g_new0(ChardevReturn, 1);
     if (CHARDEV_IS_PTY(chr)) {
         ret->pty = g_strdup(chr->filename + 4);
--
2.30.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/4] chardev/char.c: Always pass id to chardev_new

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

Always pass the id to chardev_new, since it is needed to register
the yank instance for the chardev. Also, after checking that
nothing calls chardev_new with id=NULL, assert() that id!=NULL.

This fixes a crash when using chardev-change to change a chardev
to chardev-socket, which attempts to register a yank instance.
This in turn tries to dereference the NULL-pointer.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Li Zhang <li.zhang@cloud.ionos.com>
---
 chardev/char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 48f321b3e1..75993f903f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -967,6 +967,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
     bool be_opened = true;

     assert(g_str_has_prefix(typename, "chardev-"));
+    assert(id);

     obj = object_new(typename);
     chr = CHARDEV(obj);
@@ -1095,12 +1096,11 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
         return NULL;
     }

-    chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
+    chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
                           backend, chr->gcontext, errp);
     if (!chr_new) {
         return NULL;
     }
-    chr_new->label = g_strdup(id);

     if (chr->be_open && !chr_new->be_open) {
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
--
2.30.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 4/4] chardev: Fix yank with the chardev-change case

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 7407 bytes --]

When changing from chardev-socket (which supports yank) to
chardev-socket again, it fails, because the new chardev attempts
to register a new yank instance. This in turn fails, as there
still is the yank instance from the current chardev. Also,
the old chardev shouldn't unregister the yank instance when it
is freed.

To fix this, now the new chardev only registers a yank instance if
the current chardev doesn't support yank and thus hasn't registered
one already. Also, when the old chardev is freed, it now only
unregisters the yank instance if the new chardev doesn't need it.

If the initialization of the new chardev fails, it still has
chr->handover_yank_instance set and won't unregister the yank
instance when it is freed.

s->registered_yank is always true here, as chardev-change only works
on user-visible chardevs and those are guraranteed to register a
yank instance as they are initialized via
chardev_new()
 qemu_char_open()
  cc->open() (qmp_chardev_open_socket()).

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Tested-by: Li Zhang <li.zhang@cloud.ionos.com>
---
 chardev/char-socket.c  | 20 +++++++++++++++++---
 chardev/char.c         | 35 ++++++++++++++++++++++++++++-------
 include/chardev/char.h |  3 +++
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1d455ecca4..daa89fe5d1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1126,7 +1126,13 @@ static void char_socket_finalize(Object *obj)
     }
     g_free(s->tls_authz);
     if (s->registered_yank) {
-        yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+        /*
+         * In the chardev-change special-case, we shouldn't unregister the yank
+         * instance, as it still may be needed.
+         */
+        if (!chr->handover_yank_instance) {
+            yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+        }
     }

     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -1424,8 +1430,14 @@ static void qmp_chardev_open_socket(Chardev *chr,
         qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
     }

-    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
-        return;
+    /*
+     * In the chardev-change special-case, we shouldn't register a new yank
+     * instance, as there already may be one.
+     */
+    if (!chr->handover_yank_instance) {
+        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+            return;
+        }
     }
     s->registered_yank = true;

@@ -1567,6 +1579,8 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);

+    cc->supports_yank = true;
+
     cc->parse = qemu_chr_parse_socket;
     cc->open = qmp_chardev_open_socket;
     cc->chr_wait_connected = tcp_chr_wait_connected;
diff --git a/chardev/char.c b/chardev/char.c
index 75993f903f..398f09df19 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -39,6 +39,7 @@
 #include "qemu/option.h"
 #include "qemu/id.h"
 #include "qemu/coroutine.h"
+#include "qemu/yank.h"

 #include "chardev-internal.h"

@@ -266,6 +267,7 @@ static void char_init(Object *obj)
 {
     Chardev *chr = CHARDEV(obj);

+    chr->handover_yank_instance = false;
     chr->logfd = -1;
     qemu_mutex_init(&chr->chr_write_lock);

@@ -959,6 +961,7 @@ void qemu_chr_set_feature(Chardev *chr,
 static Chardev *chardev_new(const char *id, const char *typename,
                             ChardevBackend *backend,
                             GMainContext *gcontext,
+                            bool handover_yank_instance,
                             Error **errp)
 {
     Object *obj;
@@ -971,6 +974,7 @@ static Chardev *chardev_new(const char *id, const char *typename,

     obj = object_new(typename);
     chr = CHARDEV(obj);
+    chr->handover_yank_instance = handover_yank_instance;
     chr->label = g_strdup(id);
     chr->gcontext = gcontext;

@@ -1004,7 +1008,7 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
         id = genid;
     }

-    chr = chardev_new(id, typename, backend, gcontext, errp);
+    chr = chardev_new(id, typename, backend, gcontext, false, errp);
     if (!chr) {
         return NULL;
     }
@@ -1032,7 +1036,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     }

     chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                      backend, NULL, errp);
+                      backend, NULL, false, errp);
     if (!chr) {
         return NULL;
     }
@@ -1057,9 +1061,10 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                   Error **errp)
 {
     CharBackend *be;
-    const ChardevClass *cc;
+    const ChardevClass *cc, *cc_new;
     Chardev *chr, *chr_new;
     bool closed_sent = false;
+    bool handover_yank_instance;
     ChardevReturn *ret;

     chr = qemu_chr_find(id);
@@ -1091,13 +1096,20 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
         return NULL;
     }

-    cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
-    if (!cc) {
+    cc = CHARDEV_GET_CLASS(chr);
+    cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
+    if (!cc_new) {
         return NULL;
     }

-    chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                          backend, chr->gcontext, errp);
+    /*
+     * The new chardev should not register a yank instance if the current
+     * chardev has registered one already.
+     */
+    handover_yank_instance = cc->supports_yank && cc_new->supports_yank;
+
+    chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
+                          backend, chr->gcontext, handover_yank_instance, errp);
     if (!chr_new) {
         return NULL;
     }
@@ -1121,6 +1133,15 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
         return NULL;
     }

+    /* change successfull, clean up */
+    chr_new->handover_yank_instance = false;
+
+    /*
+     * When the old chardev is freed, it should not unregister the yank
+     * instance if the new chardev needs it.
+     */
+    chr->handover_yank_instance = handover_yank_instance;
+
     object_unparent(OBJECT(chr));
     object_property_add_child(get_chardevs_root(), chr_new->label,
                               OBJECT(chr_new));
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 4181a2784a..7c0444f90d 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -65,6 +65,8 @@ struct Chardev {
     char *filename;
     int logfd;
     int be_open;
+    /* used to coordinate the chardev-change special-case: */
+    bool handover_yank_instance;
     GSource *gsource;
     GMainContext *gcontext;
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
@@ -251,6 +253,7 @@ struct ChardevClass {
     ObjectClass parent_class;

     bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
+    bool supports_yank;
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);

     void (*open)(Chardev *chr, ChardevBackend *backend,
--
2.30.2

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/4] yank: Add chardev tests and fixes

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Tue, 23 Mar 2021 18:57:32 +0100
Lukas Straub <lukasstraub2@web.de> wrote:

> Hello Everyone,
> These patches increase test coverage for yank, add tests and fix bugs and
> crashes in yank in combination with chardev-change.
> Please Review.
> 
> Regards,
> Lukas Straub
> 
> Changes:
> -v3:
>  -Base on <cover.1616521341.git.lukasstraub2@web.de>
>   ([PATCH 0/2] yank: Always link full yank code)
>  -Drop patch 1 (tests: Use the normal yank code instead of stubs in relevant tests)
> 
> -v2:
>  -test: add license
>  -test: factorize testcases to a single function
>  -test: test chardev_change with initialization of new chardev failing
>  -fix chardev_change with initialization of new chardev failing
>  -add reviewed-by and tested-by tags
> 
> Based-on: <cover.1616521341.git.lukasstraub2@web.de>
> ([PATCH 0/2] yank: Always link full yank code)
> 
> Lukas Straub (4):
>   tests: Add tests for yank with the chardev-change case
>   chardev/char.c: Move object_property_try_add_child out of chardev_new
>   chardev/char.c: Always pass id to chardev_new
>   chardev: Fix yank with the chardev-change case
> 
>  MAINTAINERS            |   1 +
>  chardev/char-socket.c  |  20 +++-
>  chardev/char.c         |  77 +++++++++++-----
>  include/chardev/char.h |   3 +
>  tests/unit/meson.build |   3 +-
>  tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 276 insertions(+), 29 deletions(-)
>  create mode 100644 tests/unit/test-yank.c
> 
> --
> 2.30.2

Ping. It would be cool if I could submit a PR for this for rc1.

Regards,
Lukas Straub

-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]