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

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

[John Snow]

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.

Tighten the loop condition to coax Coverity into dropping
its false positive.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 migration/block-dirty-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
         }
 
-- 
2.17.2

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

[Vladimir Sementsov-Ogievskiy]

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.

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?

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.

> 
> Tighten the loop condition to coax Coverity into dropping
> its false positive.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>


> ---
>   migration/block-dirty-bitmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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);
>           }
>   
> 


-- 
Best regards,
Vladimir

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

[John Snow]

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

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

--js

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

[John Snow]

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

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

--js

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

[Eric Blake]

[-- 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 --]

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

[Eric Blake]

[-- 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 --]

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

[John Snow]

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

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

[John Snow]

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