| Message ID | 1548705688-1027522-2-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-img info lists bitmap directory entries | expand |
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 <andrey.shinkevich@virtuozzo.com> > --- > > +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags) > +{ > + Qcow2BitmapInfoFlagsList *list = NULL; > + Qcow2BitmapInfoFlagsList **plist = &list; > + > + if (flags & BME_FLAG_IN_USE) { > + Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1); > + entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE; > + *plist = entry; > + plist = &entry->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 "<none>" 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 <eblake@redhat.com>
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 <andrey.shinkevich@virtuozzo.com> > --- > 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, &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: "); 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
>> >> 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 >
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, &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. 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
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, &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. > > 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 to put additional notice for human-format user.
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, &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. >> >> 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..
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, &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. > > > > 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 disk/qcow2 error", written by you in the v4 thread. I don't think "may be sad" is a good justification for non-standard behaviour. If we can't load the bitmaps, the image is broken. And if the image is broken, the rest of the information may be invalid, too. > 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. ...or that you're running an old QEMU version. I'm not sure if making old QEMU versions and image errors look the same is a good move. Almost worse than making the QAPI interface confusing, however, is that we throw away the real error message without giving it to the user. > 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 to put additional notice for human-format > user. Even for QMP, a log message on stderr is better than nothing. Kevin
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, &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. >>> >>> 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 disk/qcow2 error", written > by you in the v4 thread. > > I don't think "may be sad" is a good justification for non-standard > behaviour. If we can't load the bitmaps, the image is broken. And if the > image is broken, the rest of the information may be invalid, too. So, you mean that we go wrong way. And it was my idea. That is sad too. Than, seems like we should add errp to the function and to the full stack down to qmp_query_block. And drop extra partial-success qapi logic. > >> 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. > > ...or that you're running an old QEMU version. I'm not sure if making > old QEMU versions and image errors look the same is a good move. No, for old versions we return empty list, to show that there were no errors. > > Almost worse than making the QAPI interface confusing, however, is that > we throw away the real error message without giving it to the user. > >> 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 to put additional notice for human-format >> user. > > Even for QMP, a log message on stderr is better than nothing. > Agreed.
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, &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. > >>> > >>> 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 disk/qcow2 error", written > > by you in the v4 thread. > > > > I don't think "may be sad" is a good justification for non-standard > > behaviour. If we can't load the bitmaps, the image is broken. And if the > > image is broken, the rest of the information may be invalid, too. > > So, you mean that we go wrong way. And it was my idea. That is sad too. > > Than, seems like we should add errp to the function and to the full stack > down to qmp_query_block. And drop extra partial-success qapi logic. I'm not necessarily saying that it's the wrong way, though possibly it is wrong indeed. But ignoring some kind of failures is special, so what I was looking for were comments or documentation to explain the reason behind it at least. Now from the replies I got so far it looks to me that possibly the reasons are not only undocumented, but we might actually not have any. > >> 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. > > > > ...or that you're running an old QEMU version. I'm not sure if making > > old QEMU versions and image errors look the same is a good move. > > No, for old versions we return empty list, to show that there were no errors. You mean old image format versions? I'm talking about old QEMU versions that don't even know the 'bitmaps' field. A QMP client would have to parse the schema to understand whether a missing 'bitmaps' field means that it's talking to an old QEMU (then 'bitmaps' doesn't exist in the schema), or that an error happened (then the field does exist in the schema). Looking at the schema is not impossible, so if we require this for a good reason, it's okay. But it's not trivial either. If we really want to go with this semantics, we should probably mention both the old and the new meaning in the QAPI documentation, with the recommendation that you parse the schema to determine the meaning of a missing 'bitmaps' field. Kevin
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, &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. >>>>> >>>>> 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 disk/qcow2 error", written >>> by you in the v4 thread. >>> >>> I don't think "may be sad" is a good justification for non-standard >>> behaviour. If we can't load the bitmaps, the image is broken. And if the >>> image is broken, the rest of the information may be invalid, too. >> >> So, you mean that we go wrong way. And it was my idea. That is sad too. >> >> Than, seems like we should add errp to the function and to the full stack >> down to qmp_query_block. And drop extra partial-success qapi logic. > > I'm not necessarily saying that it's the wrong way, though possibly it > is wrong indeed. > > But ignoring some kind of failures is special, so what I was looking for > were comments or documentation to explain the reason behind it at least. > Now from the replies I got so far it looks to me that possibly the > reasons are not only undocumented, but we might actually not have any. > >>>> 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. >>> >>> ...or that you're running an old QEMU version. I'm not sure if making >>> old QEMU versions and image errors look the same is a good move. >> >> No, for old versions we return empty list, to show that there were no errors. > > You mean old image format versions? yes. I'm talking about old QEMU versions > that don't even know the 'bitmaps' field. hmm. Yes, that's a problem, which we didn't considered. > > A QMP client would have to parse the schema to understand whether a > missing 'bitmaps' field means that it's talking to an old QEMU (then > 'bitmaps' doesn't exist in the schema), or that an error happened (then > the field does exist in the schema). > > Looking at the schema is not impossible, so if we require this for a > good reason, it's okay. But it's not trivial either. If we really want > to go with this semantics, we should probably mention both the old and > the new meaning in the QAPI documentation, with the recommendation that > you parse the schema to determine the meaning of a missing 'bitmaps' > field. Hm, now I think that if we really face the case when we need partial success of info querying, it should be additional optional parameter which enables it, like skip-failed=true (default false) or something, which is more clear than parsing the schema. And, which we can add later if needed.
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, &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. >>>>>> >>>>>> 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 disk/qcow2 error", written >>>> by you in the v4 thread. >>>> >>>> I don't think "may be sad" is a good justification for non-standard >>>> behaviour. If we can't load the bitmaps, the image is broken. And if the >>>> image is broken, the rest of the information may be invalid, too. >>> >>> So, you mean that we go wrong way. And it was my idea. That is sad too. >>> >>> Than, seems like we should add errp to the function and to the full stack >>> down to qmp_query_block. And drop extra partial-success qapi logic. >> >> I'm not necessarily saying that it's the wrong way, though possibly it >> is wrong indeed. >> >> But ignoring some kind of failures is special, so what I was looking for >> were comments or documentation to explain the reason behind it at least. >> Now from the replies I got so far it looks to me that possibly the >> reasons are not only undocumented, but we might actually not have any. >> >>>>> 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. >>>> >>>> ...or that you're running an old QEMU version. I'm not sure if making >>>> old QEMU versions and image errors look the same is a good move. >>> >>> No, for old versions we return empty list, to show that there were no errors. >> >> You mean old image format versions? > > yes. > > I'm talking about old QEMU versions >> that don't even know the 'bitmaps' field. > > hmm. Yes, that's a problem, which we didn't considered. > >> >> A QMP client would have to parse the schema to understand whether a >> missing 'bitmaps' field means that it's talking to an old QEMU (then >> 'bitmaps' doesn't exist in the schema), or that an error happened (then >> the field does exist in the schema). >> >> Looking at the schema is not impossible, so if we require this for a >> good reason, it's okay. But it's not trivial either. If we really want >> to go with this semantics, we should probably mention both the old and >> the new meaning in the QAPI documentation, with the recommendation that >> you parse the schema to determine the meaning of a missing 'bitmaps' >> field. > > Hm, now I think that if we really face the case when we need partial success > of info querying, it should be additional optional parameter which enables it, like > skip-failed=true (default false) or something, which is more clear than parsing the > schema. And, which we can add later if needed. > > Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and bdrv_query_image_info skip these errors, in same manner as for formats which don't support bdrv_get_specific_info. Looks like a good moment to fix this too, do you agree?
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, &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. > >>>>>> > >>>>>> 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 disk/qcow2 error", written > >>>> by you in the v4 thread. > >>>> > >>>> I don't think "may be sad" is a good justification for non-standard > >>>> behaviour. If we can't load the bitmaps, the image is broken. And if the > >>>> image is broken, the rest of the information may be invalid, too. > >>> > >>> So, you mean that we go wrong way. And it was my idea. That is sad too. > >>> > >>> Than, seems like we should add errp to the function and to the full stack > >>> down to qmp_query_block. And drop extra partial-success qapi logic. > >> > >> I'm not necessarily saying that it's the wrong way, though possibly it > >> is wrong indeed. > >> > >> But ignoring some kind of failures is special, so what I was looking for > >> were comments or documentation to explain the reason behind it at least. > >> Now from the replies I got so far it looks to me that possibly the > >> reasons are not only undocumented, but we might actually not have any. > >> > >>>>> 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. > >>>> > >>>> ...or that you're running an old QEMU version. I'm not sure if making > >>>> old QEMU versions and image errors look the same is a good move. > >>> > >>> No, for old versions we return empty list, to show that there were no errors. > >> > >> You mean old image format versions? > > > > yes. > > > > I'm talking about old QEMU versions > >> that don't even know the 'bitmaps' field. > > > > hmm. Yes, that's a problem, which we didn't considered. > > > >> > >> A QMP client would have to parse the schema to understand whether a > >> missing 'bitmaps' field means that it's talking to an old QEMU (then > >> 'bitmaps' doesn't exist in the schema), or that an error happened (then > >> the field does exist in the schema). > >> > >> Looking at the schema is not impossible, so if we require this for a > >> good reason, it's okay. But it's not trivial either. If we really want > >> to go with this semantics, we should probably mention both the old and > >> the new meaning in the QAPI documentation, with the recommendation that > >> you parse the schema to determine the meaning of a missing 'bitmaps' > >> field. > > > > Hm, now I think that if we really face the case when we need partial success > > of info querying, it should be additional optional parameter which enables it, like > > skip-failed=true (default false) or something, which is more clear than parsing the > > schema. And, which we can add later if needed. > > Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks > will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and > bdrv_query_image_info skip these errors, in same manner as for formats which don't support > bdrv_get_specific_info. > > Looks like a good moment to fix this too, do you agree? Yes, once we add an Error **errp to .bdrv_get_specific_info, returning errors for failing qcrypto_block_get_info() calls as well (both in crypto and qcow2) makes sense to me. Or in fact, remove the errp parameter from qcrypto_block_get_info(). It seems to never return errors. Kevin
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, &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. >>>>>>>> >>>>>>>> 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 disk/qcow2 error", written >>>>>> by you in the v4 thread. >>>>>> >>>>>> I don't think "may be sad" is a good justification for non-standard >>>>>> behaviour. If we can't load the bitmaps, the image is broken. And if the >>>>>> image is broken, the rest of the information may be invalid, too. >>>>> >>>>> So, you mean that we go wrong way. And it was my idea. That is sad too. >>>>> >>>>> Than, seems like we should add errp to the function and to the full stack >>>>> down to qmp_query_block. And drop extra partial-success qapi logic. >>>> >>>> I'm not necessarily saying that it's the wrong way, though possibly it >>>> is wrong indeed. >>>> >>>> But ignoring some kind of failures is special, so what I was looking for >>>> were comments or documentation to explain the reason behind it at least. >>>> Now from the replies I got so far it looks to me that possibly the >>>> reasons are not only undocumented, but we might actually not have any. >>>> >>>>>>> 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. >>>>>> >>>>>> ...or that you're running an old QEMU version. I'm not sure if making >>>>>> old QEMU versions and image errors look the same is a good move. >>>>> >>>>> No, for old versions we return empty list, to show that there were no errors. >>>> >>>> You mean old image format versions? >>> >>> yes. >>> >>> I'm talking about old QEMU versions >>>> that don't even know the 'bitmaps' field. >>> >>> hmm. Yes, that's a problem, which we didn't considered. >>> >>>> >>>> A QMP client would have to parse the schema to understand whether a >>>> missing 'bitmaps' field means that it's talking to an old QEMU (then >>>> 'bitmaps' doesn't exist in the schema), or that an error happened (then >>>> the field does exist in the schema). >>>> >>>> Looking at the schema is not impossible, so if we require this for a >>>> good reason, it's okay. But it's not trivial either. If we really want >>>> to go with this semantics, we should probably mention both the old and >>>> the new meaning in the QAPI documentation, with the recommendation that >>>> you parse the schema to determine the meaning of a missing 'bitmaps' >>>> field. >>> >>> Hm, now I think that if we really face the case when we need partial success >>> of info querying, it should be additional optional parameter which enables it, like >>> skip-failed=true (default false) or something, which is more clear than parsing the >>> schema. And, which we can add later if needed. >> >> Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks >> will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and >> bdrv_query_image_info skip these errors, in same manner as for formats which don't support >> bdrv_get_specific_info. >> >> Looks like a good moment to fix this too, do you agree? > > Yes, once we add an Error **errp to .bdrv_get_specific_info, returning > errors for failing qcrypto_block_get_info() calls as well (both in > crypto and qcow2) makes sense to me. > > Or in fact, remove the errp parameter from qcrypto_block_get_info(). It > seems to never return errors. > > Kevin > Dear Kevin, Thank you very much for your collaboration. Your suggestion makes the code change even simpler. I have modified the patches. Please review a new version #10.
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 <andrey.shinkevich@virtuozzo.com> >> --- > >> >> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags) >> +{ >> + Qcow2BitmapInfoFlagsList *list = NULL; >> + Qcow2BitmapInfoFlagsList **plist = &list; >> + >> + if (flags & BME_FLAG_IN_USE) { >> + Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1); >> + entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE; >> + *plist = entry; >> + plist = &entry->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 "<none>" 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 <eblake@redhat.com> > 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.
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 = &list; + + if (flags & BME_FLAG_IN_USE) { + Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1); + entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE; + *plist = entry; + plist = &entry->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 = &list; + + 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 = &obj->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 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 */ }; } else if (s->qcow_version == 3) { + Qcow2BitmapInfoList *bitmaps; + Error *local_err = NULL; + + bitmaps = qcow2_get_bitmap_info_list(bs, &local_err); *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); } else { /* if this assertion fails, this probably means a new version was * added without having it covered here */ diff --git a/block/qcow2.h b/block/qcow2.h index 438a1de..13e8964 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -684,6 +684,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, void **refcount_table, int64_t *refcount_table_size); bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, + Error **errp); int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); diff --git a/qapi/block-core.json b/qapi/block-core.json index 91685be..3352562 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -69,6 +69,10 @@ # @encrypt: details about encryption parameters; only set if image # is encrypted (since 2.10) # +# @bitmaps: A list of qcow2 bitmap details (possibly empty, such as for +# v2 images which do not support bitmaps); absent if bitmap +# information could not be obtained (since 4.0) +# # Since: 1.7 ## { 'struct': 'ImageInfoSpecificQCow2', @@ -77,7 +81,8 @@ '*lazy-refcounts': 'bool', '*corrupt': 'bool', 'refcount-bits': 'int', - '*encrypt': 'ImageInfoSpecificQCow2Encryption' + '*encrypt': 'ImageInfoSpecificQCow2Encryption', + '*bitmaps': ['Qcow2BitmapInfo'] } } ## @@ -454,6 +459,41 @@ 'status': 'DirtyBitmapStatus'} } ## +# @Qcow2BitmapInfoFlags: +# +# An enumeration of flags that a bitmap can report to the user. +# +# @in-use: The bitmap was not saved correctly and may be inconsistent. +# +# @auto: The bitmap must reflect all changes of the virtual disk by any +# application that would write to this qcow2 file. +# +# Since: 4.0 +## +{ 'enum': 'Qcow2BitmapInfoFlags', + 'data': ['in-use', 'auto'] } + +## +# @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 +# +# Since: 4.0 +## +{ 'struct': 'Qcow2BitmapInfo', + 'data': {'name': 'str', 'granularity': 'uint32', + 'flags': ['Qcow2BitmapInfoFlags'], + '*unknown-flags': 'uint32' } } + +## # @BlockLatencyHistogramInfo: # # Block latency histogram. diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out index af623cf..d0438b2 100644 --- a/tests/qemu-iotests/060.out +++ b/tests/qemu-iotests/060.out @@ -18,6 +18,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false + bitmaps: 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' ] class TestQCow3NotLazy(TestQemuImgInfo): '''Testing a qcow2 version 3 image with lazy refcounts disabled''' img_options = 'compat=1.1,lazy_refcounts=off' - json_compare = { 'compat': '1.1', 'lazy-refcounts': False, + json_compare = { 'compat': '1.1', 'lazy-refcounts': False, 'bitmaps': [], 'refcount-bits': 16, 'corrupt': False } - human_compare = [ 'compat: 1.1', 'lazy refcounts: false', + human_compare = [ 'compat: 1.1', 'lazy refcounts: false', 'bitmaps:', 'refcount bits: 16', 'corrupt: false' ] class TestQCow3Lazy(TestQemuImgInfo): '''Testing a qcow2 version 3 image with lazy refcounts enabled''' img_options = 'compat=1.1,lazy_refcounts=on' - json_compare = { 'compat': '1.1', 'lazy-refcounts': True, + json_compare = { 'compat': '1.1', 'lazy-refcounts': True, 'bitmaps': [], 'refcount-bits': 16, 'corrupt': False } - human_compare = [ 'compat: 1.1', 'lazy refcounts: true', + human_compare = [ 'compat: 1.1', 'lazy refcounts: true', 'bitmaps:', 'refcount bits: 16', 'corrupt: false' ] class TestQCow3NotLazyQMP(TestQMP): @@ -112,7 +112,7 @@ class TestQCow3NotLazyQMP(TestQMP): with lazy refcounts enabled''' img_options = 'compat=1.1,lazy_refcounts=off' qemu_options = 'lazy-refcounts=on' - compare = { 'compat': '1.1', 'lazy-refcounts': False, + compare = { 'compat': '1.1', 'lazy-refcounts': False, 'bitmaps': [], 'refcount-bits': 16, 'corrupt': False } @@ -121,7 +121,7 @@ class TestQCow3LazyQMP(TestQMP): with lazy refcounts disabled''' img_options = 'compat=1.1,lazy_refcounts=on' qemu_options = 'lazy-refcounts=off' - compare = { 'compat': '1.1', 'lazy-refcounts': True, + compare = { 'compat': '1.1', 'lazy-refcounts': True, 'bitmaps': [], 'refcount-bits': 16, 'corrupt': False } TestImageInfoSpecific = None diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out index 0ce18c0..cfae9c4 100644 --- a/tests/qemu-iotests/082.out +++ b/tests/qemu-iotests/082.out @@ -18,6 +18,7 @@ cluster_size: 4096 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false @@ -30,6 +31,7 @@ cluster_size: 8192 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false @@ -276,6 +278,7 @@ cluster_size: 4096 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false @@ -287,6 +290,7 @@ cluster_size: 8192 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false @@ -529,6 +533,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false @@ -540,6 +545,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false + bitmaps: refcount bits: 16 corrupt: false @@ -551,6 +557,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 16 corrupt: false diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out index adb805c..aeee3c6 100644 --- a/tests/qemu-iotests/198.out +++ b/tests/qemu-iotests/198.out @@ -36,6 +36,7 @@ image: json:{"encrypt.key-secret": "sec0", "driver": "IMGFMT", "file": {"driver" file format: IMGFMT virtual size: 16M (16777216 bytes) Format specific information: + bitmaps: encrypt: ivgen alg: plain64 hash alg: sha256 @@ -79,6 +80,7 @@ file format: IMGFMT virtual size: 16M (16777216 bytes) backing file: TEST_DIR/t.IMGFMT.base Format specific information: + bitmaps: encrypt: ivgen alg: plain64 hash alg: sha256 diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out index 91f4db5..9758569 100644 --- a/tests/qemu-iotests/206.out +++ b/tests/qemu-iotests/206.out @@ -19,6 +19,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false + bitmaps: refcount bits: 16 corrupt: false @@ -41,6 +42,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false + bitmaps: refcount bits: 16 corrupt: false @@ -63,6 +65,7 @@ cluster_size: 2097152 Format specific information: compat: 1.1 lazy refcounts: true + bitmaps: refcount bits: 1 corrupt: false @@ -86,6 +89,7 @@ backing file: TEST_IMG.base backing file format: IMGFMT Format specific information: compat: 0.10 + bitmaps: refcount bits: 16 === Successful image creation (encrypted) === @@ -103,6 +107,7 @@ cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false + bitmaps: refcount bits: 16 encrypt: ivgen alg: plain64
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 <andrey.shinkevich@virtuozzo.com> --- 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(-)