From patchwork Tue Mar 30 18:13:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Lukas Straub X-Patchwork-Id: 12173497 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D724DC433DB for ; Tue, 30 Mar 2021 18:20:00 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 64E0F619CB for ; Tue, 30 Mar 2021 18:20:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 64E0F619CB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=web.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:41122 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lRIy7-0007az-G9 for qemu-devel@archiver.kernel.org; Tue, 30 Mar 2021 14:19:59 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54386) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lRIry-00044X-JN for qemu-devel@nongnu.org; Tue, 30 Mar 2021 14:13:38 -0400 Received: from mout.web.de ([212.227.15.3]:35357) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lRIrv-0001rz-5X for qemu-devel@nongnu.org; Tue, 30 Mar 2021 14:13:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=dbaedf251592; t=1617128013; bh=xPDIAsYxOfhAIS0WEPtnmy532i2hm0fwDkYFf2Zze6E=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:In-Reply-To:References; b=dLQVQEm5Sz9cMCKLt+9ASvueFd8KCUtGBAD3BEkdAWY8Qomkj1sRA9ienx+PSLfjx p0QsfAR9qIjEZMduX6k89k/zdHAxhMU4KNcNRwFdgLK3EXYm9pGB1CzrRpkhJn4eLQ zRNdonLDjzAb04ycjQAyQN0ms3fLS547pXezBQnA= X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from gecko.fritz.box ([88.130.61.86]) by smtp.web.de (mrweb003 [213.165.67.108]) with ESMTPSA (Nemesis) id 0M2MZY-1lk4jp0CEm-00s6CS; Tue, 30 Mar 2021 20:13:33 +0200 Date: Tue, 30 Mar 2021 20:13:31 +0200 From: Lukas Straub To: qemu-devel Subject: [PATCH v8 3/4] chardev: Fix yank with the chardev-change case Message-ID: <9637888d7591d2971975188478bb707299a1dc04.1617127849.git.lukasstraub2@web.de> In-Reply-To: References: MIME-Version: 1.0 X-Provags-ID: V03:K1:O6BdTSpJ/3LgT/3FUtPW2L5cG0mljf19j9h0si/eJHzTsxq24Xe dKJATwCtfsX5VycSvwDELcXjTDG2lp1rH9IHJVeCk4lqezC95hu7XyX9g+or/p0E+Mrw1N9 faPxGm9G5NXLM5tQ8j+cpWO/o2VjbKDPLCKPfG9wPaek5K1EAcfwDmQu8m1Z3w2NIQNg/qS 5K4l2X9W5nHxpaAF+xs6A== X-UI-Out-Filterresults: notjunk:1;V03:K0:XV/mD+NlFX8=:TwV3C5L622lDx6B5lE1cod 1oFShM3sTlhUzQBroWIkbwtoxbGMoIFZemp8JYfXPTgtqMB5h/Ij/27OAYxAnr+g6B5YgqQ9b pcHy+LciwosNnwiQ0upOT87O2hT+VTKNFsTnTrLCV4oLLmmEk/R4faYaM0Vp4BFJBk3PkhIYR /duyGLiRno7LgronPWOtxoCtWJbJSRNCy2d4V3itKqPIzrLI7qzSzoOp0NOwMKKrhITe1EQKy /gPmrk6nVlzF417KPt/VwB9nrlbbmZ0/W/IaWT/iC07MMWS67dwptBW9s4W69zZHzT942iWNG omAl4wqBhO3fsFyl0DebPvDhVLODUjpi6Ll5nhNtI+fCTNM8Pf+o+6hreM4msItzzRSMmmxdC 8tIr5i6FKE52e1fT7gJ3KUnTIUC0STdFPFMz/hqy9tVyTVXqbdMsVpce3hC/jQLqJ+1TCGpAC 60WSEy/g+NVGYSfyQ7Z/c2YU1Y73Td7DrInT1/4YT6MKmVXLGbpd8ckDNTgKVa/EyaHRa6TjH nUm50S98cxRuyZromNQIzn8R7H+Mie0ENTh5U848kH1sCOR+x5ghxfNOf9xIHtn6yqsn8sI/K UvOgyUInhnhTYHwYjksr317xAiXgRyYmxKiIb1gY1hslKPIjqcsIKT10EkqplB4LrgAmnmCIj ncHAqJPHFAuuw3x3PQuk5O+UQpta4UTSHMmFx0S3+vZR4dUjD9cStDa3YgCjIIFseAWaJsdO1 fyLeia34cqMvNf9NxZ0nEllSGl1fTKT6zvAMevqamv68ZNEFvbrTGDmyQuCr5FvbPh7cuTkik Tr5RLWTEoefaagIKTs7dRj+nSzBZ401ZGVoSDfAT+lep2+JITgKgO4zkhGw/+qjZiFlXvwJzM PodJpSs0U00aAnE4t+4RlGrNFoQw+K2Ky07F+jeC1u89yNZmUr8WAJV8Mv6lBmgDN+Jpj1y2t nVj25DtwsXf24t+DfEGYsLA6kYIZLKf8hT5oBI6ZNjAwUUN56piL37UFBWwRapdWqBe85HwF2 6xts3Wo2oDbQQ1jsmsSbEA8PdraKGdo/k6hiQWUxUN5/Qxk02851Cm+YRc41hD1LQtp0Y0unh Fg+GkrFHMzl9m+idKCpV77jtKnR92xqf7+Ycpt1phaCp27NM8tskyZbXvriSDD0ZkxVgjJscg FhaJ0SiX2g4VEwN6G2T86bXCrwqa0ayfHmqpyvPPfbhDyRscOpruYESO7jXGjgNqPnTzg= 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+qemu-devel=archiver.kernel.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,