From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 19BCEC77B7C for ; Wed, 10 May 2023 12:34:02 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pwj0l-0000CQ-Gh; Wed, 10 May 2023 08:33:39 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pwj0j-0000C2-96 for qemu-devel@nongnu.org; Wed, 10 May 2023 08:33:37 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pwj0h-0001iS-Cd for qemu-devel@nongnu.org; Wed, 10 May 2023 08:33:36 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683722014; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=abo54/GvBj3N/D1pOTd6eX6xBgWWBXLqhdmn5FKYX+k=; b=N6SUcTXhmvMOcgX+KfNxsEC21QFm91yOpkCDw3JUOYhe6A8JoIE5ZFKoM+OPLKSCu0GE9E sQi4MjrlTY/VryanuCGuh/Y+ITliPxXXQDy0mEM8wIBQ6AyyEiawUqFKY5E9i7wwCNzpnl zxzjTBb61qFRrDoKFr3fkmhYInthic4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-313-y3XedZgxPVudSkYADpKkDQ-1; Wed, 10 May 2023 08:33:33 -0400 X-MC-Unique: y3XedZgxPVudSkYADpKkDQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C6CA388B774 for ; Wed, 10 May 2023 12:33:32 +0000 (UTC) Received: from blackfin.pond.sub.org (unknown [10.39.192.121]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A3C9540C2076 for ; Wed, 10 May 2023 12:33:32 +0000 (UTC) Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 68D6221E6924; Wed, 10 May 2023 14:33:30 +0200 (CEST) From: Markus Armbruster To: Daniel P. =?utf-8?Q?Berrang=C3=A9?= Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Paolo Bonzini Subject: Re: [PATCH] chardev: report the handshake error References: <20230510072531.3937189-1-marcandre.lureau@redhat.com> <877ctg7csj.fsf@pond.sub.org> <87sfc45vak.fsf@pond.sub.org> Date: Wed, 10 May 2023 14:33:30 +0200 In-Reply-To: ("Daniel P. =?utf-8?Q?Berrang?= =?utf-8?Q?=C3=A9=22's?= message of "Wed, 10 May 2023 11:43:14 +0100") Message-ID: <87edno5pt1.fsf@pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 Received-SPF: pass client-ip=170.10.129.124; envelope-from=armbru@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Daniel P. Berrang=C3=A9 writes: > On Wed, May 10, 2023 at 12:34:59PM +0200, Markus Armbruster wrote: >> Daniel P. Berrang=C3=A9 writes: >>=20 >> > On Wed, May 10, 2023 at 11:31:40AM +0200, Markus Armbruster wrote: >> >> marcandre.lureau@redhat.com writes: >> >>=20 >> >> > From: Marc-Andr=C3=A9 Lureau >> >> > >> >> > This can help to debug connection issues. >> >> > >> >> > Related to: >> >> > https://bugzilla.redhat.com/show_bug.cgi?id=3D2196182 >> >> > >> >> > Signed-off-by: Marc-Andr=C3=A9 Lureau >> >> > --- >> >> > chardev/char-socket.c | 12 ++++++++++-- >> >> > 1 file changed, 10 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c >> >> > index 8c58532171..e8e3a743d5 100644 >> >> > --- a/chardev/char-socket.c >> >> > +++ b/chardev/char-socket.c >> >> > @@ -742,8 +742,12 @@ static void tcp_chr_websock_handshake(QIOTask = *task, gpointer user_data) >> >> > { >> >> > Chardev *chr =3D user_data; >> >> > SocketChardev *s =3D user_data; >> >> > + Error *err =3D NULL; >> >> >=20=20 >> >> > - if (qio_task_propagate_error(task, NULL)) { >> >> > + if (qio_task_propagate_error(task, &err)) { >> >> > + error_reportf_err(err, >> >> > + "websock handshake of character device %= s failed: ", >> >> > + chr->label); >> >>=20 >> >> Code smell: reports an error without failing the function. >> >>=20 >> >> Should it be a warning instead? >> > >> > Well it isn't a warning, this is a fatal error wrt continued use >> > of the chardev >> > >> > Not failing the function is expected in this particular code >> > pattern. These tcp_chr_(tls,websock)_handshake functions are >> > callbacks that are used to handle an async operations progress. >> > From the caller's POV, it doesn't matter whether there is an >> > error or success. It is upto this function to do whatever is >> > required based on the status, hence the call to disconnect >> > the chardev on error: >> > >> >> > tcp_chr_disconnect(chr); >>=20 >> Can this asynchronous task be started from QMP? > > Yes, from chardev-add. > >> If yes, how is this error reported back to the QMP client? > > It isn't, as chardev-add has already completed and returned > "success" to the client at this point IIRC. chardev-add's documentation doesn't even hint at this. It should. Is there really no need for the QMP client to know? "QMP command mererly kicks off a task, returns success before the task is done, and while the task can still fail" isn't unusual. When the task can take a long / unbounded time, it's necessary to keep QMP available. We have a few flavors of such commands, mostly for historical reasons. There are ad hoc solutions like "command kicks off, event on successful completion". If you're lucky, there's even "event on unsuccessful completion". Example: device_del, DEVICE_DELETED, DEVICE_UNPLUG_GUEST_ERROR. The latter is a recent addition. A much better developed solution is the Job abstraction. Provides commands to query and control jobs in flight, and an event on status change. Any error from the asynchronous part gets propagated to the (synchronous) query. Migration is another long-running task, and a world of its own. I wish it was a Job instead. When we add another asynchronous task, and decide against use of Jobs for whatever reasons, we should at least make our ad hoc solution as good as the better existing ad hoc solutions: properly documented, and with suitable error reporting.