Re: [Qemu-devel] [PATCH] migration/block-dirty-bitmap: Silence coverity CID 1390625

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

On 4/30/19 6:08 PM, John Snow wrote:
> 
> 
> On 11/20/18 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 16.11.2018 21:43, John Snow wrote:
>>> Coverity warns that backing_bs() could give us a NULL pointer, which
>>> we then use without checking that it isn't.
>>>
>>> In our loop condition, we check bs && bs->drv as a point of habit, but
>>> by nature of the block graph, we cannot have null bs pointers here.
>>>
>>> This loop skips only implicit nodes, which always have children, so
>>> this loop should never encounter a null value.
>>
> 
> I let this drop again :)
> 
>> You mean, always have backing (not file for ex.)? Should we at least add a comment
>> near "bool implicit;" that the node must have backing..
>>
>> Do we have filters, using 'file' child instead of backing, will we want to auto insert them, and therefore mark them with implicit=true?
>>
> 
> I actually have no idea. I guess this is the sort of thing we actually
> really want a dedicated kind of API for. "Find first non-filter" seems
> like a common use case that we'd want.
> 
> [But maybe I'll avoid this problem.]

Max has already tried to tackle that problem:
https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01713.html

> 
>> And one more thing:
>> So, it's looks like a wrong way to search for all block-nodes, instead of looping through backing chain to the first not-implicit bds, we must recursively explore the whole block graph, to find _all_ the bitmaps.
>>
> 
> Looking at this again after not having done so for so long -- I guess
> that bdrv_first/bdrv_next only iterate over *top level* BDSes and not
> any children thereof. You're right, even the method here isn't quite
> correct. We want to find ALL nodes, wherever they are.
> 
> query_named_block_nodes uses an implementation in block.c to accomplish
> this because the API is not public.... or, it wasn't, but it looks like
> we have bdrv_next_all_states now, and we could use this to just find ALL
> of the bdrv nodes.
> 
> Ehm.... let me send something a little more RFC-caliber that should
> address your concern (as well as Peter's) here.

Max's series also tries to improve how we visit nodes when determining
which bitmaps to find.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]