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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4412 bytes --]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]