Re: [Qemu-block] [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 <address@hidden>
> Signed-off-by: John Snow <address@hidden>


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