[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 05/27] hw/xen: Watches on XenStore transactions
From: |
David Woodhouse |
Subject: |
Re: [PULL 05/27] hw/xen: Watches on XenStore transactions |
Date: |
Tue, 20 Jun 2023 18:57:08 +0100 |
User-agent: |
Evolution 3.44.4-0ubuntu1 |
On Tue, 2023-06-20 at 13:19 +0100, Peter Maydell wrote:
> On Fri, 2 Jun 2023 at 18:06, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > On Tue, 2 May 2023 at 18:08, Peter Maydell
> > <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse
> > > <dwmw2@infradead.org> wrote:
> > > >
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
>
> > > 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