Re: [Qemu-block] [PATCH v9 1/2] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

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 <address@hidden>
>> ---
> 
>>   
>> +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 <address@hidden>
> 

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