>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index c66f949..0fde98c 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -38,6 +38,7 @@
>> #include "qapi/qmp/qstring.h"
>> #include "sysemu/block-backend.h"
>> #include "qemu/cutils.h"
>> +#include "qemu/error-report.h"
>>
>> BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>> BlockDriverState *bs, Error **errp)
>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function
>> func_fprintf, void *f,
>>
>> if (info->has_format_specific) {
>> func_fprintf(f, "Format specific information:\n");
>> + if (info->format_specific &&
>> + info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>> + info->format_specific->u.qcow2.data->has_bitmaps == false) {
>> + warn_report("Failed to load bitmap list");
>> + }
>> bdrv_image_info_specific_dump(func_fprintf, f,
>> info->format_specific);
>> }
>> }
>
> Is it really necessary to introduce qcow2 specific knowledge in
> block/qapi.c (where it definitely doesn't belong), just to emit a
> warning?
>
> Why can't you print the warning in qcow2_get_specific_info()?
Dear Kevin,
The reason behind the idea to move the warning to qapi is that we
wouldn't like to print that in case of JSON format.
>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 4897aba..07b99ee 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific
>> *qcow2_get_specific_info(BlockDriverState *bs)
>> *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>> .compat = g_strdup("0.10"),
>> .refcount_bits = s->refcount_bits,
>> + .has_bitmaps = true, /* To handle error check properly */
>> + .bitmaps = NULL, /* Unsupported for version 2 */
>
> .has_bitmaps = false would be nicer if the format doesn't even support
> bitmaps. You only need this here because you put the warning in the
> wrong place.
>
>> };
>> } else if (s->qcow_version == 3) {
>> + Qcow2BitmapInfoList *bitmaps;
>> + Error *local_err = NULL;
>> +
>> + bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>
> Here you even had a good error message to print...
>
>> *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>> .compat = g_strdup("1.1"),
>> .lazy_refcounts = s->compatible_features &
>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific
>> *qcow2_get_specific_info(BlockDriverState *bs)
>> QCOW2_INCOMPAT_CORRUPT,
>> .has_corrupt = true,
>> .refcount_bits = s->refcount_bits,
>> + .has_bitmaps = !local_err,
>> + .bitmaps = bitmaps,
>> };
>> + /*
>> + * If an error occurs in obtaining bitmaps, ignore
>> + * it to show other QCOW2 specific information.
>> + */
>> + error_free(local_err);
>
> ...but you decided to discard it.
>
> How about you do this here:
>
> warn_reportf_err(local_err, "Failed to load bitmap list: ");
In case of JSON format, we can fail here.
It will happen in the current implementation of the new test #239.
>
> And then get rid of the code in block/qapi.c and the version 2 path?
>
> Actually, should this really only be a warning, or in fact an error?
> What's the case where we expect that loading the bitmap list can fail,
> but the management tool doesn't need to know that and we just want to
> log a message?
>
>> } else {
>> /* if this assertion fails, this probably means a new version was
>> * added without having it covered here */
>
> Kevin
>
--
With the best regards,
Andrey Shinkevich
