diff mbox series

[v4] qemu-img info lists bitmap directory entries

Message ID 1544176849-899477-1-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series [v4] qemu-img info lists bitmap directory entries | expand

Commit Message

Andrey Shinkevich Dec. 7, 2018, 10 a.m. UTC
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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v4:
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        |  8 ++++++++
 block/qcow2.h        |  2 ++
 qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 105 insertions(+), 1 deletion(-)

Comments

Eric Blake Dec. 7, 2018, 4:20 p.m. UTC | #1
On 12/7/18 4:00 AM, 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:
> 
> 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

I'm guessing you doctored an image in a hex-editor to get this 
particular output?

>              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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> v4:
> Unknown flags are checked with the mask BME_RESERVED_FLAGS.
> The code minor refactoring was made.
> 

>   
> block/qcow2-bitmap.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c        |  8 ++++++++
>   block/qcow2.h        |  2 ++
>   qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++-
>   4 files changed, 105 insertions(+), 1 deletion(-)

I'm assuming John will merge this as a bitmap-related patch; make sure 
he is in cc if you send a v5 (adding now).

> +++ b/block/qcow2.c
> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>               .refcount_bits      = s->refcount_bits,
>           };
>       } else if (s->qcow_version == 3) {
> +        Qcow2BitmapInfoList *bitmaps;
> +        Error *local_err = NULL;
> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> +        if (local_err != NULL) {
> +            error_report_err(local_err);
> +        }

Ouch. Calling error_report_err() doesn't always work in QMP context; 
better would be to plumb Error **errp back up to the caller, if getting 
this specific information can fail and we want the overall query-block 
to fail.  Or, we could decide that failure to get bitmap info is 
non-fatal, and that it was just a best-effort attempt to get more info, 
where we then ignore the failure, rather than trying to report it 
incorrectly.


> +++ b/qapi/block-core.json
> @@ -69,6 +69,8 @@
>   # @encrypt: details about encryption parameters; only set if image
>   #           is encrypted (since 2.10)
>   #
> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
> +#
>   # Since: 1.7
>   ##
>   { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +79,8 @@
>         '*lazy-refcounts': 'bool',
>         '*corrupt': 'bool',
>         'refcount-bits': 'int',
> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +      '*bitmaps': ['Qcow2BitmapInfo']

Hmm. You're omitting this field both if there are 0 bitmaps, and when it 
was a version 2 image. Is it worth including this field as a 0-length 
array when there are no bitmaps but when the image format is new enough 
to support them, or are we happy with the idea of only including the 
field when it has at least one bitmap?  The difference is whether the 
calling app can explicitly learn that there are no bitmaps (0-length 
reply) vs. the ambiguity of omitting it from the reply (missing might 
mean no bitmaps, or an error in trying to report the bitmaps, or an 
older qemu that didn't know how to report bitmaps).


> +##
> +# @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' } }

Here, you said flags will always be present, even if it is a 0-length 
array. Did you test the case where an on-disk bitmap has neither 
'in-use' nor 'auto' set (where get_bitmap_info_flags() returns NULL) to 
ensure that it indeed results in a 0-length reply and not a crash?

Otherwise, it's looking fairly good.
Vladimir Sementsov-Ogievskiy Dec. 7, 2018, 5:24 p.m. UTC | #2
07.12.2018 19:20, Eric Blake wrote:
> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>               .refcount_bits      = s->refcount_bits,
>>           };
>>       } else if (s->qcow_version == 3) {
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err != NULL) {
>> +            error_report_err(local_err);
>> +        }
> 
> Ouch. Calling error_report_err() doesn't always work in QMP context; better would be to plumb Error **errp back up to the caller, if getting this specific information can fail and we want the overall query-block to fail.  Or, we could decide that failure to get bitmap info is non-fatal, and that it was just a best-effort attempt to get more info, where we then ignore the failure, rather than trying to report it incorrectly.

Oh, yes, you are right. Strange, but  bdrv_get_specific_info lacks errp. error_abort us used above for crypto info errors.

Querying bitmaps needs disk access so it really may fail, and it may be sad to fail get any information because of repeating some disk/qcow2 error, so it's better not to abort and even not to fail qmp command here.

> 
> 
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['Qcow2BitmapInfo']
> 
> Hmm. You're omitting this field both if there are 0 bitmaps, and when it was a version 2 image. Is it worth including this field as a 0-length array when there are no bitmaps but when the image format is new enough to support them, or are we happy with the idea of only including the field when it has at least one bitmap?  The difference is whether the calling app can explicitly learn that there are no bitmaps (0-length reply) vs. the ambiguity of omitting it from the reply (missing might mean no bitmaps, or an error in trying to report the bitmaps, or an older qemu that didn't know how to report bitmaps).

Hmm, I don't like overusing .has_bitmaps as a sign of error, at least it would be very weird to document such behavior, and a undocumented trick it is not really useful.
Hmm, if we want something like this I'd prefer .has_bitmaps to be false only in case of error, so for v2 we'll have empty array. It's simpler to document.

Or we need separate cant_load_bitmaps: bool, or bitmaps should be enum of ( ['Qcow2BitmapInfo'] , {'error': 'str'} ), do we have something like this already in QAPI? This is the question about partial success of info-exporting commands.
Eric Blake Dec. 7, 2018, 5:31 p.m. UTC | #3
On 12/7/18 11:24 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2018 19:20, Eric Blake wrote:
>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>                .refcount_bits      = s->refcount_bits,
>>>            };
>>>        } else if (s->qcow_version == 3) {
>>> +        Qcow2BitmapInfoList *bitmaps;
>>> +        Error *local_err = NULL;
>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> +        if (local_err != NULL) {
>>> +            error_report_err(local_err);
>>> +        }
>>
>> Ouch. Calling error_report_err() doesn't always work in QMP context; better would be to plumb Error **errp back up to the caller, if getting this specific information can fail and we want the overall query-block to fail.  Or, we could decide that failure to get bitmap info is non-fatal, and that it was just a best-effort attempt to get more info, where we then ignore the failure, rather than trying to report it incorrectly.
> 
> Oh, yes, you are right. Strange, but  bdrv_get_specific_info lacks errp. error_abort us used above for crypto info errors.

And thus we should probably fix that - but it doesn't have to be part of 
this series.

> 
> Querying bitmaps needs disk access so it really may fail, and it may be sad to fail get any information because of repeating some disk/qcow2 error, so it's better not to abort and even not to fail qmp command here.
> 

>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>
>> Hmm. You're omitting this field both if there are 0 bitmaps, and when it was a version 2 image. Is it worth including this field as a 0-length array when there are no bitmaps but when the image format is new enough to support them, or are we happy with the idea of only including the field when it has at least one bitmap?  The difference is whether the calling app can explicitly learn that there are no bitmaps (0-length reply) vs. the ambiguity of omitting it from the reply (missing might mean no bitmaps, or an error in trying to report the bitmaps, or an older qemu that didn't know how to report bitmaps).
> 
> Hmm, I don't like overusing .has_bitmaps as a sign of error, at least it would be very weird to document such behavior, and a undocumented trick it is not really useful.
> Hmm, if we want something like this I'd prefer .has_bitmaps to be false only in case of error, so for v2 we'll have empty array. It's simpler to document.

I'm happy with v2 images reporting a 0-length array instead of omitting 
the field, especially if that means we can simply have:

old qemu: field always omitted because we didn't compute it
new qemu: field omitted if we hit an error computing it
           otherwise present (but possibly 0 length) and accurate

> 
> Or we need separate cant_load_bitmaps: bool, or bitmaps should be enum of ( ['Qcow2BitmapInfo'] , {'error': 'str'} ), do we have something like this already in QAPI? This is the question about partial success of info-exporting commands.

No, I don't see a reason to over-engineer things; query-block is already 
a behemoth without trying to jam in more details about whether 
info-exporting hit partial failure.
Andrey Shinkevich Dec. 7, 2018, 5:50 p.m. UTC | #4
On 07.12.2018 19:20, Eric Blake wrote:
> On 12/7/18 4:00 AM, 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:
>>
>> 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
>
> I'm guessing you doctored an image in a hex-editor to get this 
> particular output?
>
>>              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>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> v4:
>> Unknown flags are checked with the mask BME_RESERVED_FLAGS.
>> The code minor refactoring was made.
>>
>
>>   block/qcow2-bitmap.c | 56 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |  8 ++++++++
>>   block/qcow2.h        |  2 ++
>>   qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 105 insertions(+), 1 deletion(-)
>
> I'm assuming John will merge this as a bitmap-related patch; make sure 
> he is in cc if you send a v5 (adding now).
>
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs)
>>               .refcount_bits      = s->refcount_bits,
>>           };
>>       } else if (s->qcow_version == 3) {
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err != NULL) {
>> +            error_report_err(local_err);
>> +        }
>
> Ouch. Calling error_report_err() doesn't always work in QMP context; 
> better would be to plumb Error **errp back up to the caller, if 
> getting this specific information can fail and we want the overall 
> query-block to fail.  Or, we could decide that failure to get bitmap 
> info is non-fatal, and that it was just a best-effort attempt to get 
> more info, where we then ignore the failure, rather than trying to 
> report it incorrectly.
>
>
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['Qcow2BitmapInfo']
>
> Hmm. You're omitting this field both if there are 0 bitmaps, and when 
> it was a version 2 image. Is it worth including this field as a 
> 0-length array when there are no bitmaps but when the image format is 
> new enough to support them, or are we happy with the idea of only 
> including the field when it has at least one bitmap? The difference is 
> whether the calling app can explicitly learn that there are no bitmaps 
> (0-length reply) vs. the ambiguity of omitting it from the reply 
> (missing might mean no bitmaps, or an error in trying to report the 
> bitmaps, or an older qemu that didn't know how to report bitmaps).
>
>
>> +##
>> +# @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' } }
>
> Here, you said flags will always be present, even if it is a 0-length 
> array. Did you test the case where an on-disk bitmap has neither 
> 'in-use' nor 'auto' set (where get_bitmap_info_flags() returns NULL) 
> to ensure that it indeed results in a 0-length reply and not a crash?
In case of no flag, the output for a bitmap looks like this:

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:
             name: x
             granularity: 65536
     refcount bits: 16
     corrupt: false

The output format is generated automatically based on the json syntax.
No crash happened.
>
> Otherwise, it's looking fairly good.
>

Kindly,

Andrey Shinkevich
Vladimir Sementsov-Ogievskiy Dec. 7, 2018, 5:52 p.m. UTC | #5
07.12.2018 20:31, Eric Blake wrote:
> On 12/7/18 11:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2018 19:20, Eric Blake wrote:
>>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>>>> +++ b/block/qcow2.c
>>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>                .refcount_bits      = s->refcount_bits,
>>>>            };
>>>>        } else if (s->qcow_version == 3) {
>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>> +        Error *local_err = NULL;
>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>> +        if (local_err != NULL) {
>>>> +            error_report_err(local_err);
>>>> +        }
>>>
>>> Ouch. Calling error_report_err() doesn't always work in QMP context; better would be to plumb Error **errp back up to the caller, if getting this specific information can fail and we want the overall query-block to fail.  Or, we could decide that failure to get bitmap info is non-fatal, and that it was just a best-effort attempt to get more info, where we then ignore the failure, rather than trying to report it incorrectly.
>>
>> Oh, yes, you are right. Strange, but  bdrv_get_specific_info lacks errp. error_abort us used above for crypto info errors.
> 
> And thus we should probably fix that - but it doesn't have to be part of this series.
> 
>>
>> Querying bitmaps needs disk access so it really may fail, and it may be sad to fail get any information because of repeating some disk/qcow2 error, so it's better not to abort and even not to fail qmp command here.
>>
> 
>>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>>
>>> Hmm. You're omitting this field both if there are 0 bitmaps, and when it was a version 2 image. Is it worth including this field as a 0-length array when there are no bitmaps but when the image format is new enough to support them, or are we happy with the idea of only including the field when it has at least one bitmap?  The difference is whether the calling app can explicitly learn that there are no bitmaps (0-length reply) vs. the ambiguity of omitting it from the reply (missing might mean no bitmaps, or an error in trying to report the bitmaps, or an older qemu that didn't know how to report bitmaps).
>>
>> Hmm, I don't like overusing .has_bitmaps as a sign of error, at least it would be very weird to document such behavior, and a undocumented trick it is not really useful.
>> Hmm, if we want something like this I'd prefer .has_bitmaps to be false only in case of error, so for v2 we'll have empty array. It's simpler to document.
> 
> I'm happy with v2 images reporting a 0-length array instead of omitting the field, especially if that means we can simply have:
> 
> old qemu: field always omitted because we didn't compute it
> new qemu: field omitted if we hit an error computing it
>            otherwise present (but possibly 0 length) and accurate

anyway, we can adjust human output type of qemu-img info to catch this case and print additional error message

> 
>>
>> Or we need separate cant_load_bitmaps: bool, or bitmaps should be enum of ( ['Qcow2BitmapInfo'] , {'error': 'str'} ), do we have something like this already in QAPI? This is the question about partial success of info-exporting commands.
> 
> No, I don't see a reason to over-engineer things; query-block is already a behemoth without trying to jam in more details about whether info-exporting hit partial failure.
>
Vladimir Sementsov-Ogievskiy Dec. 7, 2018, 5:56 p.m. UTC | #6
07.12.2018 20:52, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2018 20:31, Eric Blake wrote:
>> On 12/7/18 11:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.12.2018 19:20, Eric Blake wrote:
>>>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>>>>> +++ b/block/qcow2.c
>>>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>                .refcount_bits      = s->refcount_bits,
>>>>>            };
>>>>>        } else if (s->qcow_version == 3) {
>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>> +        Error *local_err = NULL;
>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>> +        if (local_err != NULL) {
>>>>> +            error_report_err(local_err);
>>>>> +        }
>>>>
>>>> Ouch. Calling error_report_err() doesn't always work in QMP context; better would be to plumb Error **errp back up to the caller, if getting this specific information can fail and we want the overall query-block to fail.  Or, we could decide that failure to get bitmap info is non-fatal, and that it was just a best-effort attempt to get more info, where we then ignore the failure, rather than trying to report it incorrectly.
>>>
>>> Oh, yes, you are right. Strange, but  bdrv_get_specific_info lacks errp. error_abort us used above for crypto info errors.
>>
>> And thus we should probably fix that - but it doesn't have to be part of this series.
>>
>>>
>>> Querying bitmaps needs disk access so it really may fail, and it may be sad to fail get any information because of repeating some disk/qcow2 error, so it's better not to abort and even not to fail qmp command here.
>>>
>>
>>>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>>>
>>>> Hmm. You're omitting this field both if there are 0 bitmaps, and when it was a version 2 image. Is it worth including this field as a 0-length array when there are no bitmaps but when the image format is new enough to support them, or are we happy with the idea of only including the field when it has at least one bitmap?  The difference is whether the calling app can explicitly learn that there are no bitmaps (0-length reply) vs. the ambiguity of omitting it from the reply (missing might mean no bitmaps, or an error in trying to report the bitmaps, or an older qemu that didn't know how to report bitmaps).
>>>
>>> Hmm, I don't like overusing .has_bitmaps as a sign of error, at least it would be very weird to document such behavior, and a undocumented trick it is not really useful.
>>> Hmm, if we want something like this I'd prefer .has_bitmaps to be false only in case of error, so for v2 we'll have empty array. It's simpler to document.
>>
>> I'm happy with v2 images reporting a 0-length array instead of omitting the field, especially if that means we can simply have:
>>
>> old qemu: field always omitted because we didn't compute it
>> new qemu: field omitted if we hit an error computing it
>>            otherwise present (but possibly 0 length) and accurate
> 
> anyway, we can adjust human output type of qemu-img info to catch this case and print additional error message

and to make it simpler to recognize fail, it should be one thing, independent of the version, for example: omitted always means error. Or visa-versa: empty list always means error, omitted means no bitmaps or unsupported.

> 
>>
>>>
>>> Or we need separate cant_load_bitmaps: bool, or bitmaps should be enum of ( ['Qcow2BitmapInfo'] , {'error': 'str'} ), do we have something like this already in QAPI? This is the question about partial success of info-exporting commands.
>>
>> No, I don't see a reason to over-engineer things; query-block is already a behemoth without trying to jam in more details about whether info-exporting hit partial failure.
>>
> 
>
Andrey Shinkevich Dec. 10, 2018, 12:12 p.m. UTC | #7
On 07.12.2018 19:20, Eric Blake wrote:

> On 12/7/18 4:00 AM, 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:
>>
>> 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
>
> I'm guessing you doctored an image in a hex-editor to get this 
> particular output?
Actually, I hardcoded unspecified flags just to sample the output with them.
>
>
>>              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>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> v4:
>> Unknown flags are checked with the mask BME_RESERVED_FLAGS.
>> The code minor refactoring was made.
>>
>
>>   block/qcow2-bitmap.c | 56 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |  8 ++++++++
>>   block/qcow2.h        |  2 ++
>>   qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 105 insertions(+), 1 deletion(-)
>
> I'm assuming John will merge this as a bitmap-related patch; make sure 
> he is in cc if you send a v5 (adding now).
Well noted, thank you!
>
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs)
>>               .refcount_bits      = s->refcount_bits,
>>           };
>>       } else if (s->qcow_version == 3) {
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err != NULL) {
>> +            error_report_err(local_err);
>> +        }
>
> Ouch. Calling error_report_err() doesn't always work in QMP context; 
> better would be to plumb Error **errp back up to the caller, if 
> getting this specific information can fail and we want the overall 
> query-block to fail.  Or, we could decide that failure to get bitmap 
> info is non-fatal, and that it was just a best-effort attempt to get 
> more info, where we then ignore the failure, rather than trying to 
> report it incorrectly.
In my test environment, error_report_err() and warn_report_err() both 
cause printing an error message as the only output of 'qemu-img info' 
command.
>
>
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['Qcow2BitmapInfo']
>
> Hmm. You're omitting this field both if there are 0 bitmaps, and when 
> it was a version 2 image. Is it worth including this field as a 
> 0-length array when there are no bitmaps but when the image format is 
> new enough to support them, or are we happy with the idea of only 
> including the field when it has at least one bitmap? The difference is 
> whether the calling app can explicitly learn that there are no bitmaps 
> (0-length reply) vs. the ambiguity of omitting it from the reply 
> (missing might mean no bitmaps, or an error in trying to report the 
> bitmaps, or an older qemu that didn't know how to report bitmaps).
What if we add the information about printing birmaps to the QEMU-IMG 
online manual and will omit it in the command output in case of no bitmap?
>
>
>> +##
>> +# @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' } }
>
> Here, you said flags will always be present, even if it is a 0-length 
> array. Did you test the case where an on-disk bitmap has neither 
> 'in-use' nor 'auto' set (where get_bitmap_info_flags() returns NULL) 
> to ensure that it indeed results in a 0-length reply and not a crash?
>
> Otherwise, it's looking fairly good.
Thank you!
Markus Armbruster Dec. 10, 2018, 1:08 p.m. UTC | #8
Please don't use "Content-Transfer-Encoding: base64".

Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:

> On 07.12.2018 19:20, Eric Blake wrote:
>
>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
[...]
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific 
>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>               .refcount_bits      = s->refcount_bits,
>>>           };
>>>       } else if (s->qcow_version == 3) {
>>> +        Qcow2BitmapInfoList *bitmaps;
>>> +        Error *local_err = NULL;
>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> +        if (local_err != NULL) {
>>> +            error_report_err(local_err);
>>> +        }
>>
>> Ouch. Calling error_report_err() doesn't always work in QMP context; 
>> better would be to plumb Error **errp back up to the caller, if 
>> getting this specific information can fail and we want the overall 
>> query-block to fail.  Or, we could decide that failure to get bitmap 
>> info is non-fatal, and that it was just a best-effort attempt to get 
>> more info, where we then ignore the failure, rather than trying to 
>> report it incorrectly.
> In my test environment, error_report_err() and warn_report_err() both 
> cause printing an error message as the only output of 'qemu-img info' 
> command.

Eric is right, using error_report() & friends in QMP context is almost
always wrong.  Here's the issue in a bit more detail.  Consider QMP
command query-block.  Reverse call stack:

    qmp_query_block()
    bdrv_query_info()
    bdrv_block_device_info()
    bdrv_query_image_info()
    bdrv_get_specific_info()
    qcow2_get_specific_info()

The error you report in qcow2_get_specific_info() gets reported to
stderr.  It is *not* reported to the QMP client.  Why is that a good
idea?

[...]
diff mbox series

Patch

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..8d5489e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4270,6 +4270,12 @@  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
             .refcount_bits      = s->refcount_bits,
         };
     } else if (s->qcow_version == 3) {
+        Qcow2BitmapInfoList *bitmaps;
+        Error *local_err = NULL;
+        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
+        if (local_err != NULL) {
+            error_report_err(local_err);
+        }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4279,6 +4285,8 @@  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
                                   QCOW2_INCOMPAT_CORRUPT,
             .has_corrupt        = true,
             .refcount_bits      = s->refcount_bits,
+            .has_bitmaps        = !!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..9d4011c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -69,6 +69,8 @@ 
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: list of qcow2 bitmaps details (since 4.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +79,8 @@ 
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['Qcow2BitmapInfo']
   } }
 
 ##
@@ -454,6 +457,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.