hw/xen: Clarify (lack of) error handling in transaction_commit()

Commit Message

David Woodhouse June 20, 2023, 5:58 p.m. UTC

From: David Woodhouse <[e-mail]>

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 <[e-mail]>
---
 hw/i386/kvm/xenstore_impl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Paul Durrant July 26, 2023, 9:23 a.m. UTC | #1

On 20/06/2023 18:58, David Woodhouse wrote:
> From: David Woodhouse <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   hw/i386/kvm/xenstore_impl.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 

Reviewed-by: Paul Durrant <[e-mail]>

Patch

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;