From patchwork Mon Mar 29 18:55:52 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Lukas Straub X-Patchwork-Id: 1459768 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=nongnu.org (client-ip=209.51.188.17; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=web.de header.i=@web.de header.a=rsa-sha256 header.s=dbaedf251592 header.b=f1xgnHYG; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4F8MJ66ccPz9sWL for ; Tue, 30 Mar 2021 05:56:53 +1100 (AEDT) Received: from localhost ([::1]:52326 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lQx4E-0004BZ-Vo for incoming@patchwork.ozlabs.org; Mon, 29 Mar 2021 14:56:51 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60516) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lQx3X-00046X-JV for qemu-devel@nongnu.org; Mon, 29 Mar 2021 14:56:11 -0400 Received: from mout.web.de ([212.227.15.3]:45717) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lQx3M-0001Xb-Bl for qemu-devel@nongnu.org; Mon, 29 Mar 2021 14:56:07 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=dbaedf251592; t=1617044153; bh=3RvPmWNdEWl1HNZ/qZ8MozvI3ECqIyCh35GWfDndBx4=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:In-Reply-To:References; b=f1xgnHYG+vGzFVs4Xp8QM424qOaEJ5a+hvb4sztWl8207glCuX5VOc6dKyZysE6i6 HwMFrJKBt9Xg4FIIZ91pkcwJFzhLtzABm/iKXzTsqrD7NnSIVz1WwoE0qFR4tvfy96 T/Mjulz58tmquj6C0Nu+UGPo14nZmVfH6X+wpTR8= X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from gecko.fritz.box ([94.134.180.225]) by smtp.web.de (mrweb002 [213.165.67.108]) with ESMTPSA (Nemesis) id 0MY6lc-1l50LB0fFg-00UnUU; Mon, 29 Mar 2021 20:55:53 +0200 Date: Mon, 29 Mar 2021 20:55:52 +0200 From: Lukas Straub To: qemu-devel Subject: [PATCH v7 3/4] chardev: Fix yank with the chardev-change case Message-ID: <9637888d7591d2971975188478bb707299a1dc04.1617043998.git.lukasstraub2@web.de> In-Reply-To: References: MIME-Version: 1.0 X-Provags-ID: V03:K1:itv9Y6VLIbq6Y/4ZJwqZT1yfywLYdiqFyZ/X/6w0p6qbtOaQP+L 77njtAN0xSNMKwf+M9c4cTl1stL3b2c9HfgffVzVGvm2viQVYZdSLJBUiQwCXGGRFD1/V9k tL66WiUZD44NILH4RfQ56BYG4Qjt33nW3j0Dbvoy47nngzSpeDs1HAqkf4clVIWQlF/O6Dy k+xiU60hNhRERDrF/UB1g== X-UI-Out-Filterresults: notjunk:1;V03:K0:heMqhtXJiq4=:3R7Dc9+vn624hOt7+leG04 uBOsptbncgAp+TtEtKSHvAOnegdjC7/CaBOVka9SKui7Olbw3RMnw3pBoTKl8Mf6xpAZow2yG P03aviirqabzzbdnX0lZbvv58QuR+dBjaLLEEdyIsJeOQ5k98Ng4mhZ5O/ZrHxg197bXuQ/bq 5br6AGoaufM2dGXZSOjEzHzKPD23yoeckMdG+0CLuNsnuGtruY3WYZlebHplrKD6GPenh+kIz jyoC2N6Hc3zzoQ9W7b3SySSXSLeJWicKBocoMoRkMD8bddkFP4o2bodUzW1I2Vp3LUM4ttoHb Z17FvuhYpME203y52Dgu1RPxsc8TzjWufPt8G7gMBamS/fY43fIPBEiGNmXsvUK1gY2LbiPO1 U3KlfdQvtFRkprGDOWsa/sS/z4MAsEb1CZ/bzPuxbJD5cVgHotW4mdgg2WLkuRPw2rPMmKMvy fTO3BvcBGClfGwC+unGroeYKGOWE+Y1E445KGpjae5DlRpxAqbY/eTcZQtuXmV19rVlTbrcY6 Qh5xDqwt+/xW0afohTbBlhg/q28bFspOFFGgsDETeljsfBahAm+NtGoLAmkhoPheudyobNI5B cpCvCXlHkHGa+5RjF8Se0MnOXwsGRycp/gcEROs0tiEVZnLnnQkM/VdWxd0xtrHIR+Uegg01G PS64ZadqPlOa5sN3zILuyZHhi7xbNagVya+AoZtSvmJEKZumMAscbCNWP5yLr3GZ73D0Y1x3W PRPKqOjkL5a7k9+rz9AZP5/vIm3fhZ1Nq3p2inCqoJFYWgLWyhc+cPo4Twy/0D3VdV+pCCtDj 99MeKTEE00u4O8GVuXR0HIwPm6f/d8PFEZAE3UFHr2n/RVN+WQZagMuNL+oTeRfRpXZmBvgin 3+BtAk8BpjC/Rs/v7QupvNlC3iBiG9hb/AmP1A1Z0d7HPllYCCC6K88xbh0h1Cxq6ft9xuymT 85xrqXUngUTwuSXF0fDnsHJvZDeja2FiE+Lol4rV8ECKXe1/YGQVLrZEf1M4c0kgWEManw5LQ BP3xANKJxP9xuxe50NySfQJMDHJL+stie+yfih5uX569qiXJdT06v6ByT5ibH5K76UYWfTfh7 nTFHB0diQoGjeKwETVFXv9+OPRdhoadbM8LtpN82xmF+J/qCU97YLCsj0qkWx6p5XKkACKiYQ ARaYE20K6r6E32488OiCPVVYLEHLp8/6O+GMJS/+Bj1Zt2b2Z0oqiZ9aR/pnSW9mP7MLQ= Received-SPF: pass client-ip=212.227.15.3; envelope-from=lukasstraub2@web.de; helo=mout.web.de X-Spam_score_int: -24 X-Spam_score: -2.5 X-Spam_bar: -- X-Spam_report: (-2.5 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marc-Andre Lureau Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" 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 Reviewed-by: Marc-André Lureau Tested-by: Li Zhang --- chardev/char-socket.c | 20 +++++++++++++++++--- chardev/char.c | 35 ++++++++++++++++++++++++++++------- include/chardev/char.h | 3 +++ 3 files changed, 48 insertions(+), 10 deletions(-) -- 2.30.2 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,