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.shinkev...@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 <ebl...@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.
--
With the best regards,
Andrey Shinkevich
