[Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries

Andrey Shinkevich posted 1 patch 5 years, 4 months ago
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1544465367-16302-1-git-send-email-andrey.shinkevich@virtuozzo.com
There is a newer version of this series
block/qcow2-bitmap.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
block/qcow2.c        | 15 ++++++++++++++
block/qcow2.h        |  2 ++
qapi/block-core.json | 43 +++++++++++++++++++++++++++++++++++++++-
qemu-img.c           |  8 ++++++++
5 files changed, 123 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries
Posted by Andrey Shinkevich 5 years, 4 months ago
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

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
v5:
The error handling logic for the bitmaps empty list was fixed and documented.

v4:
'[PATCH v4] qemu-img info lists bitmap directory entries'.
Unknown flags are checked with the mask BME_RESERVED_FLAGS.
The code minor refactoring was made.

v3:
'[PATCH v3] qemu-img info lists bitmap directory entries'.
Now, qcow2_get_bitmap_info_list() is invoked under the condition of QCOW
version #3 to avoid memory leaks in case of QCOW version #2.
Furthermore, qcow2_get_bitmap_info_list() checks the number of existing bitmaps.
So, if no bitmap exists, no bitmap error message is printed in the output.
The data type of the bitmap 'granularity' parameter was left as 'uint32'
because bitmap_list_load() returns error if granularity_bits is grater than 31.

v2:
'[PATCH v2] qemu-img info lists bitmap directory entries'.
The targeted version of the release at 'Since' word of the comments to the new
structures changed to 4.0 in the file qapi/block-core.json.
A comment to the 'bitmaps' new member was supplied.
The 'unknown flags' parameter was introduced to indicate presence of QCOW2
bitmap unknown flags, if any.
The word 'dirty' was removed from the code and from the comments as we list all
the bitmaps.
The 'bitmaps' printed parameter was removed for the release versions earlier
than 3.x.
The example of the output was moved above the 'Signed-off-by' line.

The first version was '[PATCH] qemu-img info lists bitmap directory entries'.

 block/qcow2-bitmap.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        | 15 ++++++++++++++
 block/qcow2.h        |  2 ++
 qapi/block-core.json | 43 +++++++++++++++++++++++++++++++++++++++-
 qemu-img.c           |  8 ++++++++
 5 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index accebef..1c3b3e1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1008,6 +1008,62 @@ 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;
+}
+
+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 991d6ac..d046ab6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4270,6 +4270,19 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
             .refcount_bits      = s->refcount_bits,
         };
     } else if (s->qcow_version == 3) {
+        bool has_bitmaps;
+        Qcow2BitmapInfoList *bitmaps;
+        Error *local_err = NULL;
+
+        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
+        if (local_err) {
+            /* TODO: Report the Error up to the caller when implemented */
+            error_free(local_err);
+            /* The 'bitmaps' empty list designates a failure to get info */
+            has_bitmaps = true;
+        } else {
+            has_bitmaps = !!bitmaps;
+        }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4279,6 +4292,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
                                   QCOW2_INCOMPAT_CORRUPT,
             .has_corrupt        = true,
             .refcount_bits      = s->refcount_bits,
+            .has_bitmaps        = has_bitmaps,
+            .bitmaps            = bitmaps,
         };
     } else {
         /* if this assertion fails, this probably means a new version was
diff --git a/block/qcow2.h b/block/qcow2.h
index 8662b68..0ec2b3d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -685,6 +685,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 d4fe710..53820a5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -69,6 +69,11 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: list of qcow2 bitmaps details; the empty list designates
+#           "fail to load bitmaps" if it is passed to the caller or
+#           "no bitmaps" otherwise;
+#           unsupported for the format QCOW2 v2 (since 4.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +82,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['Qcow2BitmapInfo']
   } }
 
 ##
@@ -454,6 +460,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/qemu-img.c b/qemu-img.c
index ad04f59..627b3f5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2601,6 +2601,14 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
             blk_unref(blk);
             goto err;
         }
+        if (info->format_specific &&
+            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
+            info->format_specific->u.qcow2.data->has_bitmaps &&
+            info->format_specific->u.qcow2.data->bitmaps == NULL) {
+            warn_report("Failed to load bitmap list");
+            /* Remove 'bitmaps' field from the output */
+            info->format_specific->u.qcow2.data->has_bitmaps = false;
+        }
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries
Posted by Vladimir Sementsov-Ogievskiy 5 years, 4 months ago
10.12.2018 21:09, 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:
> 

[...]

> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4270,6 +4270,19 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>               .refcount_bits      = s->refcount_bits,
>           };
>       } else if (s->qcow_version == 3) {
> +        bool has_bitmaps;
> +        Qcow2BitmapInfoList *bitmaps;
> +        Error *local_err = NULL;
> +
> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> +        if (local_err) {
> +            /* TODO: Report the Error up to the caller when implemented */
> +            error_free(local_err);
> +            /* The 'bitmaps' empty list designates a failure to get info */
> +            has_bitmaps = true;
> +        } else {
> +            has_bitmaps = !!bitmaps;
> +        }
>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>               .compat             = g_strdup("1.1"),
>               .lazy_refcounts     = s->compatible_features &
> @@ -4279,6 +4292,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>                                     QCOW2_INCOMPAT_CORRUPT,
>               .has_corrupt        = true,
>               .refcount_bits      = s->refcount_bits,
> +            .has_bitmaps        = has_bitmaps,
> +            .bitmaps            = bitmaps,
>           };
>       } else {
>           /* if this assertion fails, this probably means a new version was

[..]

> index d4fe710..53820a5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -69,6 +69,11 @@
>   # @encrypt: details about encryption parameters; only set if image
>   #           is encrypted (since 2.10)
>   #
> +# @bitmaps: list of qcow2 bitmaps details; the empty list designates
> +#           "fail to load bitmaps" if it is passed to the caller or
> +#           "no bitmaps" otherwise;
> +#           unsupported for the format QCOW2 v2 (since 4.0)


For me, it looks simpler to declare alternative approach, assuming that absence
of the field means error, like this:

@bitmaps: optional field with uncommon semantics:
           Absence of the field means that bitmaps info query failed (which doesn't
           imply the whole query failure).
           If the field exists in query results, there were no errors, and it represents
           list of qcow2 bitmaps details. So, successful result will always use empty
           list to show that there are no bitmaps.
           Note: bitmaps are not supported before QCOW2 v3, so for elder versions
           @bitmaps will always be an empty list.


The main question here: is it a first time, we are doing something like this? If not,
we must go the existing way.

I've found the only one similar thing:
in qapi/misc.json:
# If @unavailable-features is an empty list, the CPU model is
# runnable using the current host and machine-type.
# If @unavailable-features is not present, runnability
# information for the CPU is not available.

it's not about error, however..

Interesting what is the common (most common) behavior around empty-list/absent-parameter?


Aha, one point to my semantics:
we can define required field, without '*', and it implies that there should not be any errors,
and we don't have extra options (only empty list is possible to show absence of elements).
and than, '*' shows possibility of errors (if described in spec).

and with your semantics, if we want to say in general, that empty-list = error, we'll need to
use '*' for all fields, even for thous without possible errors.

(of course, we actually can not say something in general, because, I'm afraid, that we currently
have mixed semantics around empty lists)


> +#
>   # Since: 1.7
>   ##
>   { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +82,8 @@
>         '*lazy-refcounts': 'bool',
>         '*corrupt': 'bool',
>         'refcount-bits': 'int',
> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +      '*bitmaps': ['Qcow2BitmapInfo']
>     } }
>   
>   ##
> @@ -454,6 +460,41 @@
>              'status': 'DirtyBitmapStatus'} }
>   



-- 
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries
Posted by Eric Blake 5 years, 4 months ago
On 12/10/18 12:50 PM, Vladimir Sementsov-Ogievskiy wrote:
> 10.12.2018 21:09, 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:
>>
> 
> [...]
> 
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,19 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>                .refcount_bits      = s->refcount_bits,
>>            };
>>        } else if (s->qcow_version == 3) {
>> +        bool has_bitmaps;
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err) {
>> +            /* TODO: Report the Error up to the caller when implemented */
>> +            error_free(local_err);
>> +            /* The 'bitmaps' empty list designates a failure to get info */
>> +            has_bitmaps = true;

I think you got it backwards.  I would prefer has_bitmaps = false on 
error,...

>> +        } else {
>> +            has_bitmaps = !!bitmaps;

...and an empty list when you successfully determined that there are no 
bitmaps.


>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,11 @@
>>    # @encrypt: details about encryption parameters; only set if image
>>    #           is encrypted (since 2.10)
>>    #
>> +# @bitmaps: list of qcow2 bitmaps details; the empty list designates
>> +#           "fail to load bitmaps" if it is passed to the caller or
>> +#           "no bitmaps" otherwise;
>> +#           unsupported for the format QCOW2 v2 (since 4.0)
> 
> 
> For me, it looks simpler to declare alternative approach, assuming that absence
> of the field means error, like this:
> 
> @bitmaps: optional field with uncommon semantics:
>             Absence of the field means that bitmaps info query failed (which doesn't
>             imply the whole query failure).
>             If the field exists in query results, there were no errors, and it represents
>             list of qcow2 bitmaps details. So, successful result will always use empty
>             list to show that there are no bitmaps.
>             Note: bitmaps are not supported before QCOW2 v3, so for elder versions
>             @bitmaps will always be an empty list.

I'd prefer:

@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)

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

Re: [Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries
Posted by Andrey Shinkevich 5 years, 4 months ago

On 10.12.2018 22:48, Eric Blake wrote:
> On 12/10/18 12:50 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 10.12.2018 21:09, 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:
>>>
>>
>> [...]
>>
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,19 @@ static ImageInfoSpecific 
>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>                .refcount_bits      = s->refcount_bits,
>>>            };
>>>        } else if (s->qcow_version == 3) {
>>> +        bool has_bitmaps;
>>> +        Qcow2BitmapInfoList *bitmaps;
>>> +        Error *local_err = NULL;
>>> +
>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> +        if (local_err) {
>>> +            /* TODO: Report the Error up to the caller when 
>>> implemented */
>>> +            error_free(local_err);
>>> +            /* The 'bitmaps' empty list designates a failure to get 
>>> info */
>>> +            has_bitmaps = true;
> 
> I think you got it backwards.  I would prefer has_bitmaps = false on 
> error,...
> 
>>> +        } else {
>>> +            has_bitmaps = !!bitmaps;
> 
> ...and an empty list when you successfully determined that there are no 
> bitmaps.
> 
> 
>>> +++ b/qapi/block-core.json
>>> @@ -69,6 +69,11 @@
>>>    # @encrypt: details about encryption parameters; only set if image
>>>    #           is encrypted (since 2.10)
>>>    #
>>> +# @bitmaps: list of qcow2 bitmaps details; the empty list designates
>>> +#           "fail to load bitmaps" if it is passed to the caller or
>>> +#           "no bitmaps" otherwise;
>>> +#           unsupported for the format QCOW2 v2 (since 4.0)
>>
>>
>> For me, it looks simpler to declare alternative approach, assuming 
>> that absence
>> of the field means error, like this:
>>
>> @bitmaps: optional field with uncommon semantics:
>>             Absence of the field means that bitmaps info query failed 
>> (which doesn't
>>             imply the whole query failure).
>>             If the field exists in query results, there were no 
>> errors, and it represents
>>             list of qcow2 bitmaps details. So, successful result will 
>> always use empty
>>             list to show that there are no bitmaps.
>>             Note: bitmaps are not supported before QCOW2 v3, so for 
>> elder versions
>>             @bitmaps will always be an empty list.
> 
> I'd prefer:
> 
> @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)
> 

Thank you all for your comments. The interpretation of an entity depends 
on an eye of the beholder. I am flexible on the discussed matter and 
will include your approach into v6 version. Thank you very much for your 
collaboration.
-- 
With the best regards,
Andrey Shinkevich