Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive?

[Vladimir Sementsov-Ogievskiy]

09.11.2020 18:22, Eric Blake wrote:
> On 11/9/20 1:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 07.11.2020 01:53, Peter Maydell wrote:
> > > On Fri, 6 Nov 2020 at 20:36, Eric Blake <eblake@redhat.com> wrote:
> > > > On 11/6/20 11:22 AM, Peter Maydell wrote:
> > > > > Hi; Coverity's "you usually check the return value of this function
> > > > > but you didn't do that here" heuristic has fired on the code in
> > > > > nbd/server.c:bitmap_to_extents() -- the function nbd_extent_array_add()
> > > > > is called five times in server.c, and the return value is checked
> > > > > in four of those, but not in the final call at the end of
> > > > > bitmap_to_extents(). (CID 1436125.)
> > > > >
> > > > > Is this a false positive, or should the caller be handling an
> > > > > error here ?
> > > >
> > > > False positive, but I don't mind tweaking the code to silence Coverity.
> > > > This should do it; let me know if I should turn it into a formal patch.
> > > >
> > > > diff --git i/nbd/server.c w/nbd/server.c
> > > > index d145e1a69083..377698a2ce85 100644
> > > > --- i/nbd/server.c
> > > > +++ w/nbd/server.c
> > > > @@ -2128,9 +2128,8 @@ static void bitmap_to_extents(BdrvDirtyBitmap
> > > > *bitmap,
> > > >            }
> > > >        }
> > > >
> > > > -    if (!full) {
> > > > -        /* last non dirty extent */
> > > > -        nbd_extent_array_add(es, end - start, 0);
> > > > +    if (!full && nbd_extent_array_add(es, end - start, 0) < 0) {
> > > > +        /* last non dirty extent, not a problem if array is now full */
> > > >        }
> > > >
> > > >        bdrv_dirty_bitmap_unlock(bitmap);
> > >
> > > Hmm; that looks a little odd but I guess it's a bit more
> > > documentative of the intent. Up to you whether you want
> > > to submit it as a patch or not I guess :-)
> > >
> > > thanks
> > > -- PMM
> >
> >
> > update_refcount() in block/qcow2-refcount.c is defined as
> >
> >   static int QEMU_WARN_UNUSED_RESULT update_refcount(..);
> >
> > May be, use such specifier for nbd_extent_array_add()?
>
> Adding that attribute would _force_ us to modify the code, rather than
> the current situation where we are mulling the modification merely to
> pacify Coverity's 4-out-of-5 analysis.  We don't strictly need to always
> use the return value (hence my declaration that this was a Coverity
> false positive), but declaring that we always want to use it, and fixing
> the code fallout, would indeed silence Coverity.

Oh, I thought that this macro has the opposite meaning :\

--
Best regards,
Vladimir