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

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

[Lukas Straub]

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

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

Regards,
Lukas Straub

Based-on: <20210316135907.3646901-1-armbru@redhat.com>
([PATCH] yank: Avoid linking into executables that don't want it)

Alternatively:
https://github.com/Lukey3332/qemu.git yank_next

Lukas Straub (5):
  tests: Use the normal yank code instead of stubs in relevant tests
  tests: Add tests for yank with the chardev-change
  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          |  74 ++++++++-----
 include/chardev/char.h  |   4 +
 tests/qtest/meson.build |   6 +-
 tests/unit/meson.build  |   5 +-
 tests/unit/test-yank.c  | 240 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 317 insertions(+), 33 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 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Lukas Straub]

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

Use the normal yank code instead of stubs in relevant tests to
increase coverage and to ensure that registering and unregistering
of yank instances and functions is done correctly.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 tests/qtest/meson.build | 6 +++---
 tests/unit/meson.build  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 66ee9fbf45..40e1f495f7 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
 qtests = {
   'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
   'cdrom-test': files('boot-sector.c'),
-  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
+  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
   'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
-  'migration-test': files('migration-helpers.c'),
+  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
   'pxe-test': files('boot-sector.c'),
   'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
   'tpm-crb-swtpm-test': [io, tpmemu_files],
@@ -266,7 +266,7 @@ foreach dir : target_dirs
   endif
   qtest_env.set('G_TEST_DBUS_DAEMON', meson.source_root() / 'tests/dbus-vmstate-daemon.sh')
   qtest_env.set('QTEST_QEMU_BINARY', './qemu-system-' + target_base)
-
+
   foreach test : target_qtests
     # Executables are shared across targets, declare them only the first time we
     # encounter them
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 4bfe4627ba..8ccf60af66 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -123,7 +123,7 @@ if have_system
     'test-util-sockets': ['socket-helpers.c'],
     'test-base64': [],
     'test-bufferiszero': [],
-    'test-vmstate': [migration, io]
+    'test-vmstate': [migration, io, '../../monitor/yank.c']
   }
   if 'CONFIG_INOTIFY1' in config_host
     tests += {'test-util-filemonitor': []}
@@ -135,7 +135,7 @@ if have_system
   if 'CONFIG_TSAN' not in config_host
     if 'CONFIG_POSIX' in config_host
         tests += {
-          'test-char': ['socket-helpers.c', qom, io, chardev]
+          'test-char': ['socket-helpers.c', qom, io, chardev, '../../monitor/yank.c']
         }
     endif

--
2.30.2


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

[PATCH 2/5] tests: Add tests for yank with the chardev-change

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 9011 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 | 240 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test-yank.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa024eed17..a8a7f0d1c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2817,6 +2817,7 @@ F: monitor/yank.c
 F: stubs/yank.c
 F: include/qemu/yank.h
 F: qapi/yank.json
+F: tests/unit/test-yank.c

 COLO Framework
 M: zhanghailiang <zhang.zhanghailiang@huawei.com>
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 8ccf60af66..38e5dba920 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, '../../monitor/yank.c']
+    'test-vmstate': [migration, io, '../../monitor/yank.c'],
+    'test-yank': ['socket-helpers.c', qom, io, chardev, '../../monitor/yank.c']
   }
   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..44f24c45a8
--- /dev/null
+++ b/tests/unit/test-yank.c
@@ -0,0 +1,240 @@
+#include "qemu/osdep.h"
+#include <glib/gstdio.h>
+
+#include "qemu/config-file.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/sockets.h"
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-char.h"
+#include "qapi/qmp/qdict.h"
+#include "qom/qom-qobject.h"
+#include "io/channel-socket.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "socket-helpers.h"
+#include "qapi/qapi-commands-yank.h"
+#include "qapi/qapi-types-yank.h"
+
+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_to_yank_test(gconstpointer opaque)
+{
+    SocketAddress *addr = (gpointer) opaque;
+    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, addr, 1, &error_abort);
+    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+    g_assert_nonnull(addr);
+
+    ChardevBackend old_backend = { .type = CHARDEV_BACKEND_KIND_NULL };
+    ChardevBackend new_backend = {
+            .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
+            } };
+
+    g_assert(!is_yank_instance_registered());
+
+    ret = qmp_chardev_add("chardev", &old_backend, &error_abort);
+    qapi_free_ChardevReturn(ret);
+    chr = qemu_chr_find("chardev");
+    g_assert_nonnull(chr);
+
+    g_assert(!is_yank_instance_registered());
+
+    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);
+
+    ret = qmp_chardev_change("chardev", &new_backend, &error_abort);
+    g_assert_nonnull(ret);
+    g_assert(be.chr != chr);
+    g_assert(is_yank_instance_registered());
+
+    object_unparent(OBJECT(be.chr));
+    object_unref(OBJECT(ioc));
+    qapi_free_ChardevReturn(ret);
+}
+
+static void char_change_yank_to_yank_test(gconstpointer opaque)
+{
+    SocketAddress *addr = (gpointer) opaque;
+    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, addr, 1, &error_abort);
+    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+    g_assert_nonnull(addr);
+
+    ChardevBackend backend = {
+            .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
+            } };
+
+    g_assert(!is_yank_instance_registered());
+
+    ret = qmp_chardev_add("chardev", &backend, &error_abort);
+    qapi_free_ChardevReturn(ret);
+    chr = qemu_chr_find("chardev");
+    g_assert_nonnull(chr);
+
+    g_assert(is_yank_instance_registered());
+
+    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);
+
+    ret = qmp_chardev_change("chardev", &backend, &error_abort);
+    g_assert_nonnull(ret);
+    g_assert(be.chr != chr);
+    g_assert(is_yank_instance_registered());
+
+    object_unparent(OBJECT(be.chr));
+    object_unref(OBJECT(ioc));
+    qapi_free_ChardevReturn(ret);
+}
+
+static void char_change_from_yank_test(gconstpointer opaque)
+{
+    SocketAddress *addr = (gpointer) opaque;
+    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, addr, 1, &error_abort);
+    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+    g_assert_nonnull(addr);
+
+    ChardevBackend old_backend = {
+            .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 new_backend = { .type = CHARDEV_BACKEND_KIND_NULL };
+
+    g_assert(!is_yank_instance_registered());
+
+    ret = qmp_chardev_add("chardev", &old_backend, &error_abort);
+    qapi_free_ChardevReturn(ret);
+    chr = qemu_chr_find("chardev");
+    g_assert_nonnull(chr);
+
+    g_assert(is_yank_instance_registered());
+
+    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);
+
+    ret = qmp_chardev_change("chardev", &new_backend, &error_abort);
+    g_assert_nonnull(ret);
+    g_assert(be.chr != chr);
+    g_assert(!is_yank_instance_registered());
+
+    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);
+
+    g_test_add_data_func("/yank/char_change_to_yank", &tcpaddr,
+                         char_change_to_yank_test);
+    g_test_add_data_func("/yank/char_change_yank_to_yank", &tcpaddr,
+                         char_change_yank_to_yank_test);
+    g_test_add_data_func("/yank/char_change_from_yank", &tcpaddr,
+                         char_change_from_yank_test);
+
+end:
+    return g_test_run();
+}
--
2.30.2


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

[PATCH 3/5] chardev/char.c: Move object_property_try_add_child out of chardev_new

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2733 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>
---
 chardev/char.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 97cafd6849..1584865027 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -972,7 +972,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) {
@@ -982,22 +984,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;
 }

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

     if (!id) {
@@ -1013,7 +1000,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,
@@ -1034,6 +1033,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 4/5] chardev/char.c: Always pass id to chardev_new

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1439 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>
---
 chardev/char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 1584865027..ad416c0714 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -964,6 +964,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);
@@ -1092,12 +1093,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 5/5] chardev: Fix yank with the chardev-change case

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 7116 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.

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>
---
 chardev/char-socket.c  | 20 +++++++++++++++++---
 chardev/char.c         | 32 +++++++++++++++++++++++++-------
 include/chardev/char.h |  4 ++++
 3 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c8bced76b7..8186b6a205 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1119,7 +1119,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->yank_unregister_instance) {
+            yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+        }
     }

     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -1421,8 +1427,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->yank_register_instance) {
+        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+            return;
+        }
     }
     s->registered_yank = true;

@@ -1564,6 +1576,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 ad416c0714..245f8be201 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,8 @@ static void char_init(Object *obj)
 {
     Chardev *chr = CHARDEV(obj);

+    chr->yank_register_instance = true;
+    chr->yank_unregister_instance = true;
     chr->logfd = -1;
     qemu_mutex_init(&chr->chr_write_lock);

@@ -956,6 +959,7 @@ void qemu_chr_set_feature(Chardev *chr,
 static Chardev *chardev_new(const char *id, const char *typename,
                             ChardevBackend *backend,
                             GMainContext *gcontext,
+                            bool yank_register_instance,
                             Error **errp)
 {
     Object *obj;
@@ -968,6 +972,7 @@ static Chardev *chardev_new(const char *id, const char *typename,

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

@@ -1001,7 +1006,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, true, errp);
     if (!chr) {
         return NULL;
     }
@@ -1029,7 +1034,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, true, errp);
     if (!chr) {
         return NULL;
     }
@@ -1054,7 +1059,7 @@ 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;
     ChardevReturn *ret;
@@ -1088,13 +1093,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);
+    /*
+     * Only register a yank instance if the current chardev hasn't registered
+     * one already.
+     */
+    chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
+                          backend, chr->gcontext,
+                          /* yank_register_instance = */ !cc->supports_yank,
+                          errp);
     if (!chr_new) {
         return NULL;
     }
@@ -1118,6 +1130,12 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
         return NULL;
     }

+    /*
+     * When the old chardev is freed, it should only unregister the yank
+     * instance if the new chardev doesn't need it.
+     */
+    chr->yank_unregister_instance = !cc_new->supports_yank;
+
     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..ff38bb3af0 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -65,6 +65,9 @@ struct Chardev {
     char *filename;
     int logfd;
     int be_open;
+    /* used to coordinate the chardev-change special-case: */
+    bool yank_register_instance;
+    bool yank_unregister_instance;
     GSource *gsource;
     GMainContext *gcontext;
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
@@ -251,6 +254,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 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Thomas Huth]

On 22/03/2021 00.31, Lukas Straub wrote:
> Use the normal yank code instead of stubs in relevant tests to
> increase coverage and to ensure that registering and unregistering
> of yank instances and functions is done correctly.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   tests/qtest/meson.build | 6 +++---
>   tests/unit/meson.build  | 4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 66ee9fbf45..40e1f495f7 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
>   qtests = {
>     'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>     'cdrom-test': files('boot-sector.c'),
> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
>     'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
> -  'migration-test': files('migration-helpers.c'),
> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
>     'pxe-test': files('boot-sector.c'),
>     'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
>     'tpm-crb-swtpm-test': [io, tpmemu_files],

Is this really necessary for the qtests? I can understand the change for the 
unit tests, but the qtests are separate programs where I could not imagine 
that they use the yank functions in any way?

  Thomas


PS: Please add a proper description about the yank feature to either that 
yank.c file or to include/qemu/yank.h ... I had a hard time to find out what 
this code is all about until I finally looked up your cover letter of the 
original series on the mailing list.

Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Lukas Straub]

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

On Mon, 22 Mar 2021 06:20:50 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 22/03/2021 00.31, Lukas Straub wrote:
> > Use the normal yank code instead of stubs in relevant tests to
> > increase coverage and to ensure that registering and unregistering
> > of yank instances and functions is done correctly.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >   tests/qtest/meson.build | 6 +++---
> >   tests/unit/meson.build  | 4 ++--
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 66ee9fbf45..40e1f495f7 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
> >   qtests = {
> >     'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
> >     'cdrom-test': files('boot-sector.c'),
> > -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> > +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
> >     'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
> > -  'migration-test': files('migration-helpers.c'),
> > +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
> >     'pxe-test': files('boot-sector.c'),
> >     'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
> >     'tpm-crb-swtpm-test': [io, tpmemu_files],
> 
> Is this really necessary for the qtests? I can understand the change for the 
> unit tests, but the qtests are separate programs where I could not imagine 
> that they use the yank functions in any way?

Yes, it is necessary. While the yank functions are not called in these tests,
it still checks that registering and unregistering of yank instances and
functions is done correctly. I.e. That no yank functions are registered before
the instance, that the yank instance is only unregistered after all functions
where unregistered, that the same instance is not registered twice and that
the yank instance actually exists before it is unregistered.

>   Thomas
> 
> 
> PS: Please add a proper description about the yank feature to either that 
> yank.c file or to include/qemu/yank.h ... I had a hard time to find out what 
> this code is all about until I finally looked up your cover letter of the 
> original series on the mailing list.
> 

Will do.

Regards,
Lukas Straub

-- 


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

Re: [PATCH 2/5] tests: Add tests for yank with the chardev-change

[Marc-André Lureau]

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

On Mon, Mar 22, 2021 at 3:31 AM 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 | 240 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 243 insertions(+), 1 deletion(-)
>  create mode 100644 tests/unit/test-yank.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aa024eed17..a8a7f0d1c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2817,6 +2817,7 @@ F: monitor/yank.c
>  F: stubs/yank.c
>  F: include/qemu/yank.h
>  F: qapi/yank.json
> +F: tests/unit/test-yank.c
>
>  COLO Framework
>  M: zhanghailiang <zhang.zhanghailiang@huawei.com>
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 8ccf60af66..38e5dba920 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, '../../monitor/yank.c']
> +    'test-vmstate': [migration, io, '../../monitor/yank.c'],
> +    'test-yank': ['socket-helpers.c', qom, io, chardev,
> '../../monitor/yank.c']
>    }
>    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..44f24c45a8
> --- /dev/null
> +++ b/tests/unit/test-yank.c
> @@ -0,0 +1,240 @@
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +
> +#include "qemu/config-file.h"
> +#include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qemu/sockets.h"
> +#include "chardev/char-fe.h"
> +#include "sysemu/sysemu.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-char.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qom/qom-qobject.h"
> +#include "io/channel-socket.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qapi-visit-sockets.h"
> +#include "socket-helpers.h"
> +#include "qapi/qapi-commands-yank.h"
> +#include "qapi/qapi-types-yank.h"
> +
> +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_to_yank_test(gconstpointer opaque)
> +{
> +    SocketAddress *addr = (gpointer) opaque;
> +    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, addr, 1, &error_abort);
> +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> +    g_assert_nonnull(addr);
> +
> +    ChardevBackend old_backend = { .type = CHARDEV_BACKEND_KIND_NULL };
> +    ChardevBackend new_backend = {
> +            .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
> +            } };
> +
>

It looks like you should be able to factorize the code with a common
function:
test_yank_change(first, second, first_registerd, second_registered)
or something like that.

thanks

+    g_assert(!is_yank_instance_registered());
> +
> +    ret = qmp_chardev_add("chardev", &old_backend, &error_abort);
> +    qapi_free_ChardevReturn(ret);
> +    chr = qemu_chr_find("chardev");
> +    g_assert_nonnull(chr);
> +
> +    g_assert(!is_yank_instance_registered());
> +
> +    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);
> +
> +    ret = qmp_chardev_change("chardev", &new_backend, &error_abort);
> +    g_assert_nonnull(ret);
> +    g_assert(be.chr != chr);
> +    g_assert(is_yank_instance_registered());
> +
> +    object_unparent(OBJECT(be.chr));
> +    object_unref(OBJECT(ioc));
> +    qapi_free_ChardevReturn(ret);
> +}
> +
> +static void char_change_yank_to_yank_test(gconstpointer opaque)
> +{
> +    SocketAddress *addr = (gpointer) opaque;
> +    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, addr, 1, &error_abort);
> +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> +    g_assert_nonnull(addr);
> +
> +    ChardevBackend backend = {
> +            .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
> +            } };
> +
> +    g_assert(!is_yank_instance_registered());
> +
> +    ret = qmp_chardev_add("chardev", &backend, &error_abort);
> +    qapi_free_ChardevReturn(ret);
> +    chr = qemu_chr_find("chardev");
> +    g_assert_nonnull(chr);
> +
> +    g_assert(is_yank_instance_registered());
> +
> +    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);
> +
> +    ret = qmp_chardev_change("chardev", &backend, &error_abort);
> +    g_assert_nonnull(ret);
> +    g_assert(be.chr != chr);
> +    g_assert(is_yank_instance_registered());
> +
> +    object_unparent(OBJECT(be.chr));
> +    object_unref(OBJECT(ioc));
> +    qapi_free_ChardevReturn(ret);
> +}
> +
> +static void char_change_from_yank_test(gconstpointer opaque)
> +{
> +    SocketAddress *addr = (gpointer) opaque;
> +    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, addr, 1, &error_abort);
> +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> +    g_assert_nonnull(addr);
> +
> +    ChardevBackend old_backend = {
> +            .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 new_backend = { .type = CHARDEV_BACKEND_KIND_NULL };
> +
> +    g_assert(!is_yank_instance_registered());
> +
> +    ret = qmp_chardev_add("chardev", &old_backend, &error_abort);
> +    qapi_free_ChardevReturn(ret);
> +    chr = qemu_chr_find("chardev");
> +    g_assert_nonnull(chr);
> +
> +    g_assert(is_yank_instance_registered());
> +
> +    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);
> +
> +    ret = qmp_chardev_change("chardev", &new_backend, &error_abort);
> +    g_assert_nonnull(ret);
> +    g_assert(be.chr != chr);
> +    g_assert(!is_yank_instance_registered());
> +
> +    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);
> +
> +    g_test_add_data_func("/yank/char_change_to_yank", &tcpaddr,
> +                         char_change_to_yank_test);
> +    g_test_add_data_func("/yank/char_change_yank_to_yank", &tcpaddr,
> +                         char_change_yank_to_yank_test);
> +    g_test_add_data_func("/yank/char_change_from_yank", &tcpaddr,
> +                         char_change_from_yank_test);
> +
> +end:
> +    return g_test_run();
> +}
> --
> 2.30.2
>
>

-- 
Marc-André Lureau

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

Re: [PATCH 5/5] chardev: Fix yank with the chardev-change case

[Marc-André Lureau]

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

Hi

On Mon, Mar 22, 2021 at 3:32 AM Lukas Straub <lukasstraub2@web.de> wrote:

> 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.
>
> 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>
> ---
>  chardev/char-socket.c  | 20 +++++++++++++++++---
>  chardev/char.c         | 32 +++++++++++++++++++++++++-------
>  include/chardev/char.h |  4 ++++
>  3 files changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index c8bced76b7..8186b6a205 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1119,7 +1119,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->yank_unregister_instance) {
> +            yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
> +        }
>      }
>
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> @@ -1421,8 +1427,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->yank_register_instance) {
> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
> errp)) {
> +            return;
> +        }
>      }
>      s->registered_yank = true;
>
> @@ -1564,6 +1576,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 ad416c0714..245f8be201 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,8 @@ static void char_init(Object *obj)
>  {
>      Chardev *chr = CHARDEV(obj);
>
> +    chr->yank_register_instance = true;
> +    chr->yank_unregister_instance = true;
>      chr->logfd = -1;
>      qemu_mutex_init(&chr->chr_write_lock);
>
> @@ -956,6 +959,7 @@ void qemu_chr_set_feature(Chardev *chr,
>  static Chardev *chardev_new(const char *id, const char *typename,
>                              ChardevBackend *backend,
>                              GMainContext *gcontext,
> +                            bool yank_register_instance,
>                              Error **errp)
>  {
>      Object *obj;
> @@ -968,6 +972,7 @@ static Chardev *chardev_new(const char *id, const char
> *typename,
>
>      obj = object_new(typename);
>      chr = CHARDEV(obj);
> +    chr->yank_register_instance = yank_register_instance;
>      chr->label = g_strdup(id);
>      chr->gcontext = gcontext;
>
> @@ -1001,7 +1006,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, true, errp);
>      if (!chr) {
>          return NULL;
>      }
> @@ -1029,7 +1034,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, true, errp);
>      if (!chr) {
>          return NULL;
>      }
> @@ -1054,7 +1059,7 @@ 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;
>      ChardevReturn *ret;
> @@ -1088,13 +1093,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);
> +    /*
> +     * Only register a yank instance if the current chardev hasn't
> registered
> +     * one already.
> +     */
> +    chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
> +                          backend, chr->gcontext,
> +                          /* yank_register_instance = */
> !cc->supports_yank,
> +                          errp);
>      if (!chr_new) {
>          return NULL;
>      }
> @@ -1118,6 +1130,12 @@ ChardevReturn *qmp_chardev_change(const char *id,
> ChardevBackend *backend,
>          return NULL;
>      }
>
> +    /*
> +     * When the old chardev is freed, it should only unregister the yank
> +     * instance if the new chardev doesn't need it.
> +     */
> +    chr->yank_unregister_instance = !cc_new->supports_yank;
> +
>      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..ff38bb3af0 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -65,6 +65,9 @@ struct Chardev {
>      char *filename;
>      int logfd;
>      int be_open;
> +    /* used to coordinate the chardev-change special-case: */
> +    bool yank_register_instance;
> +    bool yank_unregister_instance;
>      GSource *gsource;
>      GMainContext *gcontext;
>      DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
> @@ -251,6 +254,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,
>


Not very pleased with the change, but I can't imagine anything
better/simpler atm. Maybe someone else. So
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

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

Re: [PATCH 4/5] chardev/char.c: Always pass id to chardev_new

[Marc-André Lureau]

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

On Mon, Mar 22, 2021 at 3:31 AM Lukas Straub <lukasstraub2@web.de> wrote:

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

---
>  chardev/char.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 1584865027..ad416c0714 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -964,6 +964,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);
> @@ -1092,12 +1093,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
>
>

-- 
Marc-André Lureau

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

Re: [PATCH 3/5] chardev/char.c: Move object_property_try_add_child out of chardev_new

[Marc-André Lureau]

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

Hi

On Mon, Mar 22, 2021 at 3:31 AM Lukas Straub <lukasstraub2@web.de> wrote:

> 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>
> ---
>  chardev/char.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 97cafd6849..1584865027 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -972,7 +972,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) {
> @@ -982,22 +984,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;
>  }
>
> @@ -1006,6 +992,7 @@ Chardev *qemu_chardev_new(const char *id, const char
> *typename,
>                            GMainContext *gcontext,
>                            Error **errp)
>  {
> +    Chardev *chr;
>      g_autofree char *genid = NULL;
>
>      if (!id) {
> @@ -1013,7 +1000,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);
>

You could have added a "register" argument to avoid duplication of code
imho. But since there are only 2 callers, I guess it's fine to inline it.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

+    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,
> @@ -1034,6 +1033,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
>
>

-- 
Marc-André Lureau

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

Re: [PATCH 4/5] chardev/char.c: Always pass id to chardev_new

[Li Zhang]

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

Looks good to me. I have tested this series, it works well.
Tested-by: Li Zhang <li.zhang@cloud.ionos.com>

Thanks
Li

On Mon, Mar 22, 2021 at 12:35 AM Lukas Straub <lukasstraub2@web.de> wrote:

> 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>
> ---
>  chardev/char.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 1584865027..ad416c0714 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -964,6 +964,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);
> @@ -1092,12 +1093,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: Type: text/html, Size: 2352 bytes --]

Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Thomas Huth]

On 22/03/2021 08.35, Lukas Straub wrote:
> On Mon, 22 Mar 2021 06:20:50 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 22/03/2021 00.31, Lukas Straub wrote:
>>> Use the normal yank code instead of stubs in relevant tests to
>>> increase coverage and to ensure that registering and unregistering
>>> of yank instances and functions is done correctly.
>>>
>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>>> ---
>>>    tests/qtest/meson.build | 6 +++---
>>>    tests/unit/meson.build  | 4 ++--
>>>    2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>> index 66ee9fbf45..40e1f495f7 100644
>>> --- a/tests/qtest/meson.build
>>> +++ b/tests/qtest/meson.build
>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
>>>    qtests = {
>>>      'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>>>      'cdrom-test': files('boot-sector.c'),
>>> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
>>> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
>>>      'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>>> -  'migration-test': files('migration-helpers.c'),
>>> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
>>>      'pxe-test': files('boot-sector.c'),
>>>      'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
>>>      'tpm-crb-swtpm-test': [io, tpmemu_files],
>>
>> Is this really necessary for the qtests? I can understand the change for the
>> unit tests, but the qtests are separate programs where I could not imagine
>> that they use the yank functions in any way?
> 
> Yes, it is necessary. While the yank functions are not called in these tests,
> it still checks that registering and unregistering of yank instances and
> functions is done correctly. I.e. That no yank functions are registered before
> the instance, that the yank instance is only unregistered after all functions
> where unregistered, that the same instance is not registered twice and that
> the yank instance actually exists before it is unregistered.

Now you even confused me more. Could you elaborate a little bit? If none of 
the functions are called by the test, which part of yank.c is excercised 
here at all? Could you give a more detailed example? The only thing I could 
imagine is yank_init(), but that does not look like something we need to 
check in a qtest ?

  Thomas

Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Lukas Straub]

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

On Mon, 22 Mar 2021 17:00:23 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 22/03/2021 08.35, Lukas Straub wrote:
> > On Mon, 22 Mar 2021 06:20:50 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 22/03/2021 00.31, Lukas Straub wrote:  
> >>> Use the normal yank code instead of stubs in relevant tests to
> >>> increase coverage and to ensure that registering and unregistering
> >>> of yank instances and functions is done correctly.
> >>>
> >>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> >>> ---
> >>>    tests/qtest/meson.build | 6 +++---
> >>>    tests/unit/meson.build  | 4 ++--
> >>>    2 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> >>> index 66ee9fbf45..40e1f495f7 100644
> >>> --- a/tests/qtest/meson.build
> >>> +++ b/tests/qtest/meson.build
> >>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
> >>>    qtests = {
> >>>      'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
> >>>      'cdrom-test': files('boot-sector.c'),
> >>> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> >>> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
> >>>      'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
> >>> -  'migration-test': files('migration-helpers.c'),
> >>> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
> >>>      'pxe-test': files('boot-sector.c'),
> >>>      'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
> >>>      'tpm-crb-swtpm-test': [io, tpmemu_files],  
> >>
> >> Is this really necessary for the qtests? I can understand the change for the
> >> unit tests, but the qtests are separate programs where I could not imagine
> >> that they use the yank functions in any way?  
> > 
> > Yes, it is necessary. While the yank functions are not called in these tests,
> > it still checks that registering and unregistering of yank instances and
> > functions is done correctly. I.e. That no yank functions are registered before
> > the instance, that the yank instance is only unregistered after all functions
> > where unregistered, that the same instance is not registered twice and that
> > the yank instance actually exists before it is unregistered.  
> 
> Now you even confused me more. Could you elaborate a little bit? If none of 
> the functions are called by the test, which part of yank.c is excercised 
> here at all? Could you give a more detailed example? The only thing I could 
> imagine is yank_init(), but that does not look like something we need to 
> check in a qtest ?

Oh, sorry. I meant yank's concept of a yank function here. It works this way:
The different subsystems first register a yank instance. So in this case
when starting migration in the test, the migration code first registers a
yank instance. Then, it registers _yank functions_ with this instance, for
for example to shutdown a socket.

Now, (in the real-world qemu case) if qemu hangs, the user can use the
'yank' qmp command to call the _yank functions_ to recover.

The test doesn't test the 'yank' qmp command, but yank_register_instance,
yank_register_function (which are called by the migration code and thus
also the migration test) still check if everything is done correctly.

Regards,
Lukas Straub

>   Thomas
> 



-- 


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

Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Thomas Huth]

On 22/03/2021 18.48, Lukas Straub wrote:
> On Mon, 22 Mar 2021 17:00:23 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 22/03/2021 08.35, Lukas Straub wrote:
>>> On Mon, 22 Mar 2021 06:20:50 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>    
>>>> On 22/03/2021 00.31, Lukas Straub wrote:
>>>>> Use the normal yank code instead of stubs in relevant tests to
>>>>> increase coverage and to ensure that registering and unregistering
>>>>> of yank instances and functions is done correctly.
>>>>>
>>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>>>>> ---
>>>>>     tests/qtest/meson.build | 6 +++---
>>>>>     tests/unit/meson.build  | 4 ++--
>>>>>     2 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>>> index 66ee9fbf45..40e1f495f7 100644
>>>>> --- a/tests/qtest/meson.build
>>>>> +++ b/tests/qtest/meson.build
>>>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
>>>>>     qtests = {
>>>>>       'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>>>>>       'cdrom-test': files('boot-sector.c'),
>>>>> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
>>>>> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
>>>>>       'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>>>>> -  'migration-test': files('migration-helpers.c'),
>>>>> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
>>>>>       'pxe-test': files('boot-sector.c'),
>>>>>       'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
>>>>>       'tpm-crb-swtpm-test': [io, tpmemu_files],
>>>>
>>>> Is this really necessary for the qtests? I can understand the change for the
>>>> unit tests, but the qtests are separate programs where I could not imagine
>>>> that they use the yank functions in any way?
>>>
>>> Yes, it is necessary. While the yank functions are not called in these tests,
>>> it still checks that registering and unregistering of yank instances and
>>> functions is done correctly. I.e. That no yank functions are registered before
>>> the instance, that the yank instance is only unregistered after all functions
>>> where unregistered, that the same instance is not registered twice and that
>>> the yank instance actually exists before it is unregistered.
>>
>> Now you even confused me more. Could you elaborate a little bit? If none of
>> the functions are called by the test, which part of yank.c is excercised
>> here at all? Could you give a more detailed example? The only thing I could
>> imagine is yank_init(), but that does not look like something we need to
>> check in a qtest ?
> 
> Oh, sorry. I meant yank's concept of a yank function here. It works this way:
> The different subsystems first register a yank instance. So in this case
> when starting migration in the test, the migration code first registers a
> yank instance. Then, it registers _yank functions_ with this instance, for
> for example to shutdown a socket.

But these are the qtest, separate stand-alone programs. The migration code 
of QEMU (i.e. the code in the main "migration" folder) is not linked into 
these binaries. Doing something like:

  grep -r yank tests/qtest/migration-test

should give you zero results. Thus it IMHO does not make sense to add the 
yank.c to these tests here.

Having said that, it seems like the qos-test is linking against the chardev 
code and thus might use indirectly the yank code there. So you maybe might 
want to add it to the qos-test instead?

  Thomas

Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Lukas Straub]

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

On Tue, 23 Mar 2021 05:46:24 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 22/03/2021 18.48, Lukas Straub wrote:
> > On Mon, 22 Mar 2021 17:00:23 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 22/03/2021 08.35, Lukas Straub wrote:  
> >>> On Mon, 22 Mar 2021 06:20:50 +0100
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>      
> >>>> On 22/03/2021 00.31, Lukas Straub wrote:  
> >>>>> Use the normal yank code instead of stubs in relevant tests to
> >>>>> increase coverage and to ensure that registering and unregistering
> >>>>> of yank instances and functions is done correctly.
> >>>>>
> >>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> >>>>> ---
> >>>>>     tests/qtest/meson.build | 6 +++---
> >>>>>     tests/unit/meson.build  | 4 ++--
> >>>>>     2 files changed, 5 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> >>>>> index 66ee9fbf45..40e1f495f7 100644
> >>>>> --- a/tests/qtest/meson.build
> >>>>> +++ b/tests/qtest/meson.build
> >>>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
> >>>>>     qtests = {
> >>>>>       'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
> >>>>>       'cdrom-test': files('boot-sector.c'),
> >>>>> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> >>>>> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
> >>>>>       'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
> >>>>> -  'migration-test': files('migration-helpers.c'),
> >>>>> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
> >>>>>       'pxe-test': files('boot-sector.c'),
> >>>>>       'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
> >>>>>       'tpm-crb-swtpm-test': [io, tpmemu_files],  
> >>>>
> >>>> Is this really necessary for the qtests? I can understand the change for the
> >>>> unit tests, but the qtests are separate programs where I could not imagine
> >>>> that they use the yank functions in any way?  
> >>>
> >>> Yes, it is necessary. While the yank functions are not called in these tests,
> >>> it still checks that registering and unregistering of yank instances and
> >>> functions is done correctly. I.e. That no yank functions are registered before
> >>> the instance, that the yank instance is only unregistered after all functions
> >>> where unregistered, that the same instance is not registered twice and that
> >>> the yank instance actually exists before it is unregistered.  
> >>
> >> Now you even confused me more. Could you elaborate a little bit? If none of
> >> the functions are called by the test, which part of yank.c is excercised
> >> here at all? Could you give a more detailed example? The only thing I could
> >> imagine is yank_init(), but that does not look like something we need to
> >> check in a qtest ?  
> > 
> > Oh, sorry. I meant yank's concept of a yank function here. It works this way:
> > The different subsystems first register a yank instance. So in this case
> > when starting migration in the test, the migration code first registers a
> > yank instance. Then, it registers _yank functions_ with this instance, for
> > for example to shutdown a socket.  
> 
> But these are the qtest, separate stand-alone programs. The migration code 
> of QEMU (i.e. the code in the main "migration" folder) is not linked into 
> these binaries. Doing something like:
> 
>   grep -r yank tests/qtest/migration-test
> 
> should give you zero results. Thus it IMHO does not make sense to add the 
> yank.c to these tests here.
> 
> Having said that, it seems like the qos-test is linking against the chardev 
> code and thus might use indirectly the yank code there. So you maybe might 
> want to add it to the qos-test instead?

Ok, now I understand. In that case it doesn't matter if full yank is linked
into qtest.

Regards,
Lukas Straub

>   Thomas
> 



-- 


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