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

[Lukas Straub]

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

pgpI0MGGZyFaD.pgp
Description: OpenPGP digital signature