* nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? @ 2020-11-06 17:22 Peter Maydell 2020-11-06 20:35 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2020-11-06 17:22 UTC (permalink / raw) To: QEMU Developers 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 ? thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? 2020-11-06 17:22 nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? Peter Maydell @ 2020-11-06 20:35 ` Eric Blake 2020-11-06 22:53 ` Peter Maydell 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2020-11-06 20:35 UTC (permalink / raw) To: Peter Maydell, QEMU Developers, Vladimir Sementsov-Ogievskiy 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); -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? 2020-11-06 20:35 ` Eric Blake @ 2020-11-06 22:53 ` Peter Maydell 2020-11-09 7:17 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2020-11-06 22:53 UTC (permalink / raw) To: Eric Blake; +Cc: Vladimir Sementsov-Ogievskiy, QEMU Developers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? 2020-11-06 22:53 ` Peter Maydell @ 2020-11-09 7:17 ` Vladimir Sementsov-Ogievskiy 2020-11-09 15:22 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-11-09 7:17 UTC (permalink / raw) To: Peter Maydell, Eric Blake; +Cc: QEMU Developers 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()? -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? 2020-11-09 7:17 ` Vladimir Sementsov-Ogievskiy @ 2020-11-09 15:22 ` Eric Blake 2020-11-10 7:29 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2020-11-09 15:22 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, Peter Maydell; +Cc: QEMU Developers 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? 2020-11-09 15:22 ` Eric Blake @ 2020-11-10 7:29 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-11-10 7:29 UTC (permalink / raw) To: Eric Blake, Peter Maydell; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-10 7:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-06 17:22 nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? Peter Maydell 2020-11-06 20:35 ` Eric Blake 2020-11-06 22:53 ` Peter Maydell 2020-11-09 7:17 ` Vladimir Sementsov-Ogievskiy 2020-11-09 15:22 ` Eric Blake 2020-11-10 7:29 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).