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

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

[Peter Maydell]

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

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

[Eric Blake]

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

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

[Peter Maydell]

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

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

[Vladimir Sementsov-Ogievskiy]

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

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

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