| Message ID | 20201111163510.713855-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-5.2?] nbd: Silence Coverity false positive | expand |
On 11/11/20 8:35 AM, Eric Blake wrote: > - 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, nothing to do if array was already full */ > } Casting to (void) is another way to get rid of the warning. I dunno which makes more sense here. Definitely the comment is helpful. r~
On 11/12/20 3:04 PM, Richard Henderson wrote: > On 11/11/20 8:35 AM, Eric Blake wrote: >> - 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, nothing to do if array was already full */ >> } > > Casting to (void) is another way to get rid of the warning. > > I dunno which makes more sense here. Definitely the comment is helpful. As in: if (!full) { /* last non dirty extent, nothing to do if array is now full */ (void) nbd_extent_array_add(es, end - start, 0); } Yeah, that looks a little better. Should I post that as v2, or wait for further comments on this?
On 11/12/20 1:09 PM, Eric Blake wrote: > On 11/12/20 3:04 PM, Richard Henderson wrote: >> On 11/11/20 8:35 AM, Eric Blake wrote: >>> - 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, nothing to do if array was already full */ >>> } >> >> Casting to (void) is another way to get rid of the warning. >> >> I dunno which makes more sense here. Definitely the comment is helpful. > > As in: > > if (!full) { > /* last non dirty extent, nothing to do if array is now full */ > (void) nbd_extent_array_add(es, end - start, 0); > } Yep. > Yeah, that looks a little better. Should I post that as v2, or wait for > further comments on this? Up to you. You can have my Reviewed-by: Richard Henderson <richard.henderson@linaro.org> for either version. r~
diff --git a/nbd/server.c b/nbd/server.c index d145e1a69083..377698a2ce85 100644 --- a/nbd/server.c +++ b/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, nothing to do if array was already full */ } bdrv_dirty_bitmap_unlock(bitmap);
Coverity noticed (CID 1436125) that we check the return value of nbd_extent_array_add in most places, but not at the end of bitmap_to_extents(). The return value exists to break loops before a future iteration, so there is nothing to check if we are already done iterating. That said, a minor rearrangement to the code plus a better comment pacifies Coverity. Signed-off-by: Eric Blake <eblake@redhat.com> --- Not a show-stopper by itself for 5.2, but perhaps worth including in -rc2 if I collect other more-important patches. nbd/server.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)