From patchwork Tue Jun 20 17:58:55 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 13286313 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.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 D1970EB64D7 for ; Tue, 20 Jun 2023 17:59:18 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.552192.862127 (Exim 4.92) (envelope-from ) id 1qBfdE-0003V3-Lf; Tue, 20 Jun 2023 17:59:08 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 552192.862127; Tue, 20 Jun 2023 17:59:08 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qBfdE-0003Uw-Id; Tue, 20 Jun 2023 17:59:08 +0000 Received: by outflank-mailman (input) for mailman id 552192; Tue, 20 Jun 2023 17:59:07 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qBfdD-00030v-Ee for xen-devel@lists.xenproject.org; Tue, 20 Jun 2023 17:59:07 +0000 Received: from casper.infradead.org (casper.infradead.org [2001:8b0:10b:1236::1]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 22d269ce-0f94-11ee-8611-37d641c3527e; Tue, 20 Jun 2023 19:59:04 +0200 (CEST) Received: from [2001:8b0:10b:5:f26b:798c:7de3:5106] (helo=u3832b3a9db3152.ant.amazon.com) by casper.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1qBfd2-00DNGy-1q; Tue, 20 Jun 2023 17:58:56 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 22d269ce-0f94-11ee-8611-37d641c3527e DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=MIME-Version:Content-Type:References: In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=P0rw6j/526CasCXlDjY88i1DcnH89PB7+0min8yStY8=; b=ZpLlOTsOcbygS6lck1zF0UH1KD 7SjmlMugZb9keYTZLWbAHAqdyc2E862zUAcmk3+QRKEnmpQeVatOJ8mhnaG5Gt2x2Yn0M02uue5Th eIHFDzbUgdBN8bZy+AlB5tNskU5tWsvKjVrcqflIRVmYovUwlOYGVk8iWCDVZdI6x6nKfuPcy8ep3 rrM8p+0rswr4dpkm2UyLJegesLxB1x/us6NAXpNBKo8LF71YJ2L0tGViZHEM0p/Vu/04l11PjnKmE 1WU0SlNUj2AQ1/iIqNm/l/GYS8+eGQNUScPm+d8vW1rsc2DPjhPDkpIUXhHTG+KbkhsVX2cqNlVdY TspDN2Tw==; Message-ID: <20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org> Subject: [PATCH] hw/xen: Clarify (lack of) error handling in transaction_commit() From: David Woodhouse To: Peter Maydell Cc: qemu-devel@nongnu.org, Paolo Bonzini , Paul Durrant , Joao Martins , Ankur Arora , Stefano Stabellini , vikram.garhwal@amd.com, Anthony Perard , xen-devel@lists.xenproject.org, Juan Quintela , "Dr . David Alan Gilbert" Date: Tue, 20 Jun 2023 18:58:55 +0100 In-Reply-To: References: <20230307182707.2298618-1-dwmw2@infradead.org> <20230307182707.2298618-6-dwmw2@infradead.org> User-Agent: Evolution 3.44.4-0ubuntu1 MIME-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html From: David Woodhouse Coverity was unhappy (CID 1508359) because we didn't check the return of init_walk_op() in transaction_commit(), despite doing so at every other call site. Strictly speaking, this is a false positive since it can never fail. It only fails for invalid user input (transaction ID or path), and both of those are hard-coded to known sane values in this invocation. But Coverity doesn't know that, and neither does the casual reader of the code. Returning an error here would be weird, since the transaction *is* committed by this point; all the walk_op is doing is firing watches on the newly-committed changed nodes. So make it a g_assert(!ret), since it really should never happen. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant --- hw/i386/kvm/xenstore_impl.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/i386/kvm/xenstore_impl.c b/hw/i386/kvm/xenstore_impl.c index 305fe75519..d9732b567e 100644 --- a/hw/i386/kvm/xenstore_impl.c +++ b/hw/i386/kvm/xenstore_impl.c @@ -1022,6 +1022,7 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) { struct walk_op op; XsNode **n; + int ret; if (s->root_tx != tx->base_tx) { return EAGAIN; @@ -1032,7 +1033,16 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) s->root_tx = tx->tx_id; s->nr_nodes = tx->nr_nodes; - init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); + ret = init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); + /* + * There are two reasons why init_walk_op() may fail: an invalid tx_id, + * or an invalid path. We pass XBT_NULL and "/", and it cannot fail. + * If it does, the world is broken. And returning 'ret' would be weird + * because the transaction *was* committed, and all this tree walk is + * trying to do is fire the resulting watches on newly-committed nodes. + */ + g_assert(!ret); + op.deleted_in_tx = false; op.mutating = true;