Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
On 28/01/2019 23:43, Eric Blake wrote:
> On 1/28/19 2:01 PM, Andrey Shinkevich wrote:
>> In the 'Format specific information' section of the 'qemu-img info'
>> command output, the supplemental information about existing QCOW2
>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>
>
>>
>> As the print of the qcow2 specific information expanded by adding
>> the bitmap parameters to the 'qemu-img info' command output,
>> it requires amendment of the output benchmark in the following
>> tests: 060, 065, 082, 198, and 206.
>>
>> Signed-off-by: Andrey Shinkevich
>> ---
>
>>
>> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
>> +{
>> +Qcow2BitmapInfoFlagsList *list = NULL;
>> +Qcow2BitmapInfoFlagsList **plist =
>> +
>> +if (flags & BME_FLAG_IN_USE) {
>> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList,
>> 1);
>> +entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
>> +*plist = entry;
>> +plist = >next;
>
> This line...
>
>> +}
>> +if (flags & BME_FLAG_AUTO) {
>> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList,
>> 1);
>> +entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
>> +*plist = entry;
>> +}
>
> ...is omitted here. Harmless for now, but may cause grief if a later
> flag is added and we forget to add it in. On the other hand, I don't
> know if a static analyzer might warn about a dead assignment, so
> breaking the symmetry between the two is okay if that is the justification.
>
> Also, thinking about future flag additions, would it make any sense to
> write this code in a for loop? Something like (untested):
>
> static const struct Map {
> int bme;
> int info;
> } map[] = {
> { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
> { BME_FLAG_AUTO, QCOW2_BITMAP_INFO_FLAGS_AUTO },
> };
>
> for (i = 0; i < ARRAY_LENGTH(map); i++) {
> if (flags & map[i].bme) {
> ...; entry->value = map[i].info;
> }
>
> where adding a new bit is now a one-liner change to the definition of
> 'map' rather than a 6-line addition of a new conditional.
>
>
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: flags of the bitmap
>> +#
>> +# @unknown-flags: unspecified flags if detected
>
> Maybe:
>
> @flags: recognized flags of the bitmap
>
> @unknown-flags: any remaining flags not recognized by this qemu version
>
>
>> +++ b/tests/qemu-iotests/060.out
>> @@ -18,6 +18,7 @@ cluster_size: 65536
>> Format specific information:
>> compat: 1.1
>> lazy refcounts: false
>> +bitmaps:
>
> Hmm. I'm wondering if the human-readable output of a QAPI type with an
> optional array should output "" or something similar for a
> 0-element array, to make it obvious to the human reading the output that
> there are no bitmaps. That's not necessarily a problem in your patch;
> and may have even bigger effects to other iotests, so it should be done
> as a separate patch if we want it. But even in your patch, if we did
> that,...
>
>> refcount bits: 16
>> corrupt: true
>> can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be
>> opened read/write
>> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
>> index 8bac383..86406cb 100755
>> --- a/tests/qemu-iotests/065
>> +++ b/tests/qemu-iotests/065
>> @@ -88,23 +88,23 @@ class TestQMP(TestImageInfoSpecific):
>> class TestQCow2(TestQemuImgInfo):
>> '''Testing a qcow2 version 2 image'''
>> img_options = 'compat=0.10'
>> -json_compare = { 'compat': '0.10', 'refcount-bits': 16 }
>> -human_compare = [ 'compat: 0.10', 'refcount bits: 16' ]
>> +json_compare = { 'compat': '0.10', 'bitmaps': [], 'refcount-bits': 16 }
>> +human_compare = [ 'compat: 0.10', 'bitmaps:', 'refcount bits: 16' ]
>
> ...the human_compare dict would have to account for whatever string we
> output for an empty JSON array.
>
> I'm finding the functionality useful, though, so unless there are strong
> opinions presented on making further tweaks, I'm also fine giving this
> version as-is:
>
> Reviewed-by: Eric Blake
>
Dear Eric,
Thank you very much for your foresight and considerate approach.
I have made changes in the series and am about to send a new version #10.
Please review that.
I appreciate your collaboration.
--
With the best regards,
Andrey Shinkevich
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
On 29/01/2019 18:49, Kevin Wolf wrote:
> Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.01.2019 17:23, Kevin Wolf wrote:
Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 16:17, Kevin Wolf wrote:
>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 29.01.2019 15:38, Kevin Wolf wrote:
Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>
>>> 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, _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.
Why do you want to silently leave out the bitmap list for JSON output?
Essentially, this is the same question I asked here:
>> And
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 16:17, Kevin Wolf wrote:
> > Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 29.01.2019 15:38, Kevin Wolf wrote:
> >>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>
> >> 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, _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.
> >>>
> >>> Why do you want to silently leave out the bitmap list for JSON output?
> >>>
> >>> Essentially, this is the same question I asked here:
> >>>
> > 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?
> >>>
> >>> I don't understand why it's okay not to print bitmaps without making it
> >>> an error. Do you have a specific use case in mind for this behaviour?
> >>>
> >>
> >>
> >> We just can't print anything here, as
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
29.01.2019 17:23, Kevin Wolf wrote:
> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 16:17, Kevin Wolf wrote:
>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
29.01.2019 15:38, Kevin Wolf wrote:
> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
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, _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.
>
> Why do you want to silently leave out the bitmap list for JSON output?
>
> Essentially, this is the same question I asked here:
>
>>> 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?
>
> I don't understand why it's okay not to print bitmaps without making it
> an error. Do you have a specific use case in mind for this
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2019 17:23, Kevin Wolf wrote:
>> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 29.01.2019 16:17, Kevin Wolf wrote:
Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 15:38, Kevin Wolf wrote:
>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>
> 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, _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.
>>
>> Why do you want to silently leave out the bitmap list for JSON output?
>>
>> Essentially, this is the same question I asked here:
>>
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?
>>
>> I don't
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
> > 29.01.2019 17:23, Kevin Wolf wrote:
> >> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>> 29.01.2019 16:17, Kevin Wolf wrote:
> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 29.01.2019 15:38, Kevin Wolf wrote:
> >> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >
> > 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, _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.
> >>
> >> Why do you want to silently leave out the bitmap list for JSON output?
> >>
> >> Essentially, this is the same question I asked here:
> >>
> And then get rid of the code in block/qapi.c and the version 2 path?
>
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
29.01.2019 16:17, Kevin Wolf wrote:
> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 15:38, Kevin Wolf wrote:
>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>
>> 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, _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.
>>>
>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>
>>> Essentially, this is the same question I asked here:
>>>
> 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?
>>>
>>> I don't understand why it's okay not to print bitmaps without making it
>>> an error. Do you have a specific use case in mind for this behaviour?
>>>
>>
>>
>> We just can't print anything here, as this code is executed from qmp command.
>>
>> It was discussed, that we don't want to fail the whole query, if failed to
>> obtain bitmaps.
>
> It's obvious that you don't want to fail the query command, but I
> haven't found any explanation for _why_ you want this.
>
> The thing that is closest to an explanation
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 15:38, Kevin Wolf wrote:
> > Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>
> 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, _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.
> >
> > Why do you want to silently leave out the bitmap list for JSON output?
> >
> > Essentially, this is the same question I asked here:
> >
> >>> 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?
> >
> > I don't understand why it's okay not to print bitmaps without making it
> > an error. Do you have a specific use case in mind for this behaviour?
> >
>
>
> We just can't print anything here, as this code is executed from qmp command.
>
> It was discussed, that we don't want to fail the whole query, if failed to
> obtain bitmaps.
It's obvious that you don't want to fail the query command, but I
haven't found any explanation for _why_ you want this.
The thing that is closest to an explanation is "it may be sad to fail
get any information because of repeating some
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
29.01.2019 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2019 15:38, Kevin Wolf wrote:
>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>
> 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, _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.
>>
>> Why do you want to silently leave out the bitmap list for JSON output?
>>
>> Essentially, this is the same question I asked here:
>>
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?
>>
>> I don't understand why it's okay not to print bitmaps without making it
>> an error. Do you have a specific use case in mind for this behaviour?
>>
>
>
> We just can't print anything here, as this code is executed from qmp command.
Hmm, or we can?
Ok, for json output, it may be ok to print warnings to stderr, while formatted
json to stdout.
And for qapi, warning will go to logs, which is not bad too.
Ok, now I don't see any counterarguments..
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
29.01.2019 15:38, Kevin Wolf wrote:
> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
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, _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.
>
> Why do you want to silently leave out the bitmap list for JSON output?
>
> Essentially, this is the same question I asked here:
>
>>> 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?
>
> I don't understand why it's okay not to print bitmaps without making it
> an error. Do you have a specific use case in mind for this behaviour?
>
We just can't print anything here, as this code is executed from qmp command.
It was discussed, that we don't want to fail the whole query, if failed to
obtain bitmaps. So, to show, that there were error during bitmaps querying
we decided to use the following semantics:
empty list (has_bitmaps=true) means that there no bitmaps, and there were
no errors during bitmaps quering (if it was)
absence of the field (has_bitmaps=false) means that there _were_ errors during
bitmaps querying.
So qapi user, as well as json-output user should follow this semantics in
understanding the output. And as well as for qapi, it's incorrect to add text
messages to json output.
On the other hand, it seems reasonable
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>
> >> 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, _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.
Why do you want to silently leave out the bitmap list for JSON output?
Essentially, this is the same question I asked here:
> > 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?
I don't understand why it's okay not to print bitmaps without making it
an error. Do you have a specific use case in mind for this behaviour?
Kevin
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
>>
>> 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, _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
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Am 28.01.2019 um 21:01 hat Andrey Shinkevich geschrieben:
> In the 'Format specific information' section of the 'qemu-img info'
> command output, the supplemental information about existing QCOW2
> bitmaps will be shown, such as a bitmap name, flags and granularity:
>
> image: /vz/vmprivate/VM1/harddisk.hdd
> file format: qcow2
> virtual size: 64G (68719476736 bytes)
> disk size: 3.0M
> cluster_size: 1048576
> Format specific information:
> compat: 1.1
> lazy refcounts: true
> bitmaps:
> [0]:
> flags:
> [0]: in-use
> [1]: auto
> name: back-up1
> unknown flags: 4
> granularity: 65536
> [1]:
> flags:
> [0]: in-use
> [1]: auto
> name: back-up2
> unknown flags: 8
> granularity: 65536
> refcount bits: 16
> corrupt: false
>
> As the print of the qcow2 specific information expanded by adding
> the bitmap parameters to the 'qemu-img info' command output,
> it requires amendment of the output benchmark in the following
> tests: 060, 065, 082, 198, and 206.
>
> Signed-off-by: Andrey Shinkevich
> ---
> block/qapi.c | 6 +
> block/qcow2-bitmap.c | 64
> ++
> block/qcow2.c | 13 ++
> block/qcow2.h | 2 ++
> qapi/block-core.json | 42 +-
> tests/qemu-iotests/060.out | 1 +
> tests/qemu-iotests/065 | 16 ++--
> tests/qemu-iotests/082.out | 7 +
> tests/qemu-iotests/198.out | 2 ++
> tests/qemu-iotests/206.out | 5
> 10 files changed, 149 insertions(+), 9 deletions(-)
>
> 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()?
> 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, _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: ");
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
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
On 1/28/19 2:01 PM, Andrey Shinkevich wrote:
> In the 'Format specific information' section of the 'qemu-img info'
> command output, the supplemental information about existing QCOW2
> bitmaps will be shown, such as a bitmap name, flags and granularity:
>
>
> As the print of the qcow2 specific information expanded by adding
> the bitmap parameters to the 'qemu-img info' command output,
> it requires amendment of the output benchmark in the following
> tests: 060, 065, 082, 198, and 206.
>
> Signed-off-by: Andrey Shinkevich
> ---
>
> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
> +{
> +Qcow2BitmapInfoFlagsList *list = NULL;
> +Qcow2BitmapInfoFlagsList **plist =
> +
> +if (flags & BME_FLAG_IN_USE) {
> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList,
> 1);
> +entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
> +*plist = entry;
> +plist = >next;
This line...
> +}
> +if (flags & BME_FLAG_AUTO) {
> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList,
> 1);
> +entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
> +*plist = entry;
> +}
...is omitted here. Harmless for now, but may cause grief if a later
flag is added and we forget to add it in. On the other hand, I don't
know if a static analyzer might warn about a dead assignment, so
breaking the symmetry between the two is okay if that is the justification.
Also, thinking about future flag additions, would it make any sense to
write this code in a for loop? Something like (untested):
static const struct Map {
int bme;
int info;
} map[] = {
{ BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
{ BME_FLAG_AUTO, QCOW2_BITMAP_INFO_FLAGS_AUTO },
};
for (i = 0; i < ARRAY_LENGTH(map); i++) {
if (flags & map[i].bme) {
...; entry->value = map[i].info;
}
where adding a new bit is now a one-liner change to the definition of
'map' rather than a 6-line addition of a new conditional.
> +##
> +# @Qcow2BitmapInfo:
> +#
> +# Qcow2 bitmap information.
> +#
> +# @name: the name of the bitmap
> +#
> +# @granularity: granularity of the bitmap in bytes
> +#
> +# @flags: flags of the bitmap
> +#
> +# @unknown-flags: unspecified flags if detected
Maybe:
@flags: recognized flags of the bitmap
@unknown-flags: any remaining flags not recognized by this qemu version
> +++ b/tests/qemu-iotests/060.out
> @@ -18,6 +18,7 @@ cluster_size: 65536
> Format specific information:
> compat: 1.1
> lazy refcounts: false
> +bitmaps:
Hmm. I'm wondering if the human-readable output of a QAPI type with an
optional array should output "" or something similar for a
0-element array, to make it obvious to the human reading the output that
there are no bitmaps. That's not necessarily a problem in your patch;
and may have even bigger effects to other iotests, so it should be done
as a separate patch if we want it. But even in your patch, if we did
that,...
> refcount bits: 16
> corrupt: true
> can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be
> opened read/write
> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
> index 8bac383..86406cb 100755
> --- a/tests/qemu-iotests/065
> +++ b/tests/qemu-iotests/065
> @@ -88,23 +88,23 @@ class TestQMP(TestImageInfoSpecific):
> class TestQCow2(TestQemuImgInfo):
> '''Testing a qcow2 version 2 image'''
> img_options = 'compat=0.10'
> -json_compare = { 'compat': '0.10', 'refcount-bits': 16 }
> -human_compare = [ 'compat: 0.10', 'refcount bits: 16' ]
> +json_compare = { 'compat': '0.10', 'bitmaps': [], 'refcount-bits': 16 }
> +human_compare = [ 'compat: 0.10', 'bitmaps:', 'refcount bits: 16' ]
...the human_compare dict would have to account for whatever string we
output for an empty JSON array.
I'm finding the functionality useful, though, so unless there are strong
opinions presented on making further tweaks, I'm also fine giving this
version as-is:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
In the 'Format specific information' section of the 'qemu-img info'
command output, the supplemental information about existing QCOW2
bitmaps will be shown, such as a bitmap name, flags and granularity:
image: /vz/vmprivate/VM1/harddisk.hdd
file format: qcow2
virtual size: 64G (68719476736 bytes)
disk size: 3.0M
cluster_size: 1048576
Format specific information:
compat: 1.1
lazy refcounts: true
bitmaps:
[0]:
flags:
[0]: in-use
[1]: auto
name: back-up1
unknown flags: 4
granularity: 65536
[1]:
flags:
[0]: in-use
[1]: auto
name: back-up2
unknown flags: 8
granularity: 65536
refcount bits: 16
corrupt: false
As the print of the qcow2 specific information expanded by adding
the bitmap parameters to the 'qemu-img info' command output,
it requires amendment of the output benchmark in the following
tests: 060, 065, 082, 198, and 206.
Signed-off-by: Andrey Shinkevich
---
block/qapi.c | 6 +
block/qcow2-bitmap.c | 64 ++
block/qcow2.c | 13 ++
block/qcow2.h | 2 ++
qapi/block-core.json | 42 +-
tests/qemu-iotests/060.out | 1 +
tests/qemu-iotests/065 | 16 ++--
tests/qemu-iotests/082.out | 7 +
tests/qemu-iotests/198.out | 2 ++
tests/qemu-iotests/206.out | 5
10 files changed, 149 insertions(+), 9 deletions(-)
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);
}
}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301..ae842eb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1006,6 +1006,70 @@ fail:
return false;
}
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
+{
+Qcow2BitmapInfoFlagsList *list = NULL;
+Qcow2BitmapInfoFlagsList **plist =
+
+if (flags & BME_FLAG_IN_USE) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
+*plist = entry;
+plist = >next;
+}
+if (flags & BME_FLAG_AUTO) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
+*plist = entry;
+}
+return list;
+}
+
+/*
+ * qcow2_get_bitmap_info_list()
+ * Returns a list of QCOW2 bitmap details.
+ * In case of no bitmaps, the function returns NULL and
+ * the @errp parameter is not set (for a 0-length list in the QMP).
+ * When bitmap information can not be obtained, the function returns
+ * NULL and the @errp parameter is set (for omitting the list in QMP).
+ */
+Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
+Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+Qcow2Bitmap *bm;
+Qcow2BitmapInfoList *list = NULL;
+Qcow2BitmapInfoList **plist =
+
+if (s->nb_bitmaps == 0) {
+return NULL;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+ s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return NULL;
+}
+
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
+Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
+info->granularity = 1U << bm->granularity_bits;
+info->name = g_strdup(bm->name);
+info->flags = get_bitmap_info_flags(bm->flags);
+info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
+info->has_unknown_flags = !!info->unknown_flags;
+obj->value = info;
+*plist = obj;
+plist = >next;
+}
+
+bitmap_list_free(bm_list);
+
+return list;
+}
+
int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
Error **errp)
{
diff --git a/block/qcow2.c b/block/qcow2.c
index
