[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH] migration/block-dirty-bitmap: Silence coverity CID 1390625
Date: Wed, 1 May 2019 10:24:36 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

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:

>> 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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]