Re: [PULL 05/27] hw/xen: Watches on XenStore transactions

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Tue, 2023-06-20 at 13:19 +0100, Peter Maydell wrote:
> On Fri, 2 Jun 2023 at 18:06, Peter Maydell <peter.maydell@xxxxxxxxxx>
> wrote:
> > 
> > On Tue, 2 May 2023 at 18:08, Peter Maydell
> > <peter.maydell@xxxxxxxxxx> wrote:
> > > 
> > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse
> > > <dwmw2@xxxxxxxxxxxxx> wrote:
> > > > 
> > > > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> 
> > > 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...

Attachment: smime.p7s
Description: S/MIME cryptographic signature