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

[Eric Blake]

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