qemu-devel
[Top][All Lists]
Advanced

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

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH] migration/block-dirty-bitmap: Silence coverity CID 1390625
Date: Wed, 1 May 2019 12:31:37 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1


On 5/1/19 11:24 AM, Eric Blake wrote:
> 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
> 

OK, that's great!

>>
>>> 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.
> 

2/11 adds the "skip filters" helper I mentioned wanting, but the idea of
visiting *every* node for bitmap migration is not addressed in that series.

Therefore, I believe these series conflict, but that this one takes
precedence for this particular issue.

--js



reply via email to

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