On Tue, 2023-06-20 at 13:19 +0100, Peter Maydell wrote: > On Fri, 2 Jun 2023 at 18:06, Peter Maydell <[email protected]> > wrote: > > > > On Tue, 2 May 2023 at 18:08, Peter Maydell > > <[email protected]> wrote: > > > > > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse > > > <[email protected]> wrote: > > > > > > > > From: David Woodhouse <[email protected]> > > > > Hi; Coverity's "is there missing error handling?" > > > heuristic fired for a change in this code (CID 1508359): > > > > > > > static int transaction_commit(XenstoreImplState *s, > > > > XsTransaction *tx) > > > > { > > > > + struct walk_op op; > > > > + XsNode **n; > > > > + > > > > if (s->root_tx != tx->base_tx) { > > > > return EAGAIN; > > > > } > > > > @@ -720,10 +861,18 @@ 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); > > > > > > This is the only call to init_walk_op() which ignores its > > > return value. Intentional, or missing error handling? > > > > Hi -- I was going through the unclassified Coverity issues > > again today, and this one's still on the list. Is this a > > bug, or intentional? > > Ping^3 -- is this a false positive, or something to be fixed? > It would be nice to be able to classify the coverity issue > appropriately.
Oops, sorry for the delay.
It is arguably a false positive.
There are two cases where init_walk_op() can fail:
• It's given a transaction ID which doesn't exist. But in this case
it's actually given XBT_NULL because the transaction is *already*
committed and all we're doing is setting up a tree walk to fire
watches on the newly-committed changed nodes.
or,
• The given path is invalid. Which it isn't here because we pass a
hard-coded "/".
I was about to stick in the standard if(ret){return ret;} but the
semantics of that would be a bit bizarre.
As noted, by this point the transaction *was* committed already. So all
that gets aborted is the *watches* that were supposed to fire on
changed nodes. Returning an error in that case would be a bit weird.
So I'll go for a g_assert(!ret) with a comment about why. Patch
follows.
I shall also have another go at frowning at the soft-reset locking vs.
the timer and other code, and seeing if I win this time...
smime.p7s
Description: S/MIME cryptographic signature
