Re: [Qemu-devel] [PATCH] migration: Appease coverity, skip empty block trees

[John Snow <jsnow@redhat.com>]

On 06/22/2018 04:24 PM, Philippe Mathieu-Daudé wrote:
> On 06/22/2018 05:11 PM, John Snow wrote:
>> If a tree consists exclusively of implicit filter nodes, we might crash
>> QEMU. This configuration should not exist in practice, but if it did,
>> skipping it would be fine.
>>
>> For the purposes of debug builds, throw an assert to remind us that
>> this configuration is truly unexpected, but if it's compiled out we
>> will cope just fine.
> 
> Well... with your explanation, your patch is correct.
> But do we really want to maintain a 'debug with assert' vs 'production
> without assertions' codebase?
>

I might have misremembered, but I seem to recall that it is literally
possible to disable glib assertions. We endeavor not to, but it's
*possible*.

In this case I'd prefer to have a runtime behavior that makes sense:
skipping such trees is sensible because they won't have any bitmaps for
us to migrate.

However, this is a really bizarre block configuration that I suspect
cannot exist in practice. If it does, I'd rather not ignore it in the
development context.

> $ git grep g_assert_not_reached | egrep -v '^tests/' | wc -l
> 406
> 
> Does Coverity require all these 406 lines to behave with a
> "configuration [that] should not exist in practice"?
> 
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  migration/block-dirty-bitmap.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 3bafbbdc4c..02725293dd 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -287,6 +287,10 @@ static int init_dirty_bitmap_migration(void)
>>          while (bs && bs->drv && bs->implicit) {
>>              bs = backing_bs(bs);
>>          }
>> +        if (!bs) {
>> +            g_assert_not_reached();
>> +            continue;
> 
> Can we choose one or the other?
> 

Ask someone more familiar with assert policy and a fondness for paint hues.

--js

>> +        }
>>  
>>          for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>               bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>