|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: nbd: bitmap_to_extents() calls nbd_extent_array_add() without checking return value: coverity false positive? |
Date: | Tue, 10 Nov 2020 10:29:55 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.1 |
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 -- PMMupdate_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
[Prev in Thread] | Current Thread | [Next in Thread] |