On 16 November 2018 at 03:28, John Snow <js...@redhat.com> wrote:
> I looked again. I think Vladimir's patch will shut up Coverity for sure,
> feel free to apply it if you want this out of your hair.
>
> Stefan suggests the following, however;
>
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5e90f44c2f..00c068fda3 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -284,7 +284,7 @@ static int init_dirty_bitmap_migration(void)
> const char *drive_name = bdrv_get_device_or_node_name(bs);
>
> /* skip automatically inserted nodes */
> - while (bs && bs->drv && bs->implicit) {
> + while (bs->drv && bs->implicit) {
> bs = backing_bs(bs);
> }
>
>
> that by removing the assumption that bs could ever be null here (it
> shouldn't), that we'll coax coverity into not warning anymore. I don't
> know if that will work, because backing_bs can theoretically return NULL
> and might convince coverity there's a problem. In practice it won't be.
>
> I don't know how to check this to see if Stefan's suggestion is appropriate.
>
> For such a small, trivial issue though, just merge this and be done with
> it, in my opinion. If you want to take this fix directly as a "build
> fix" I wouldn't object.
Personally I think the main benefit of this sort of Coverity
warning is that it nudges you to work through and figure out
whether something really can be NULL or not. Stefan's fix
will satisfy Coverity, because what it's complaining about
is that the code in one place assumes a pointer can't be NULL
but in another place does have handling for NULL: it is the
inconsistency that triggers the warning. If backing_bs()
can't return NULL (ie if you would be happy in theory to put
an assert() in after the call) then I think Stefan's fix is
better. If it can return NULL ever then Vladimir's fix is
what you want.
thanks
-- PMM
