Hi
On Mon, Mar 22, 2021 at 3:32 AM Lukas Straub <lukasstra...@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 <lukasstra...@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.lur...@redhat.com>
--
Marc-André Lureau
