Re: [Qemu-devel] [Qemu-block] [PATCH] migration/block-dirty-bitmap: fix Coverity CID1390625

[Peter Maydell]

On 16 November 2018 at 16:16, John Snow <address@hidden> wrote:
> On 11/16/18 5:04 AM, Peter Maydell wrote:
>> 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.

> I really don't think it can, but I don't actually know how to verify it
> or to convince Coverity of the same. Stefan's suggestion seems most
> appropriate if it actually does calm Coverity down.

I think it will quieten Coverity; if it doesn't, then at that point
we can happily mark the issue a false-positive. But as I say what
Coverity is really identifying here is "you checked whether
this pointer was NULL, but then there's a code path forward
to a place where you used it without checking" -- so removing
the unnecessary check will make it happy.

> (Is it mechanically possible to violate this? That's very hard to audit
> and promise for you. There are always bugs lurking somewhere else.)

If you believe by design that it can't be NULL, then we're OK:
if it ever turns out that it isn't, then we will get a nice
easy-to-debug SEGV when we dereference the NULL pointer,
and we can find out then what bug resulted in our design
assumption being broken.

> Stefan's suggestion is probably most appropriate, *if* it actually
> shushes Coverity. I'll send you a small patch and you can find out? I
> don't want to task offload but I also genuinely don't know if that will
> hint to coverity strongly enough that this is a false positive.

Coverity runs only on git master, so fixes have to go into
master to be tested, unfortunately.

thanks
-- PMM