migration/block-dirty-bitmap: Silence coverity CID 1390625

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

Signed-off-by John Snow <[email protected]>

---
  migration/block-dirty-bitmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

migration/block-dirty-bitmap.c +1 -1

index 5e90f44c2f..00c068fda3 100644
@@ -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);
          }
 

Quoted 8 lines · 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.

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.

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 8 lines · 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.

I let this drop again :)

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 22 lines · John Snow

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 8 lines · 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.

I let this drop again :)

Quoted 5 lines

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?

Quoted 5 lines

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

Quoted 15 lines

Quoted 4 lines

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.

Quoted 2 lines

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.