On 29/01/2019 18:49, Kevin Wolf wrote:
> Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.01.2019 17:23, Kevin Wolf wrote:
>>>> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 29.01.2019 16:17, Kevin Wolf wrote:
>>>>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>> 29.01.2019 15:38, Kevin Wolf wrote:
>>>>>>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>>>>>>>> index c66f949..0fde98c 100644
>>>>>>>>>>> --- a/block/qapi.c
>>>>>>>>>>> +++ b/block/qapi.c
>>>>>>>>>>> @@ -38,6 +38,7 @@
>>>>>>>>>>> #include "qapi/qmp/qstring.h"
>>>>>>>>>>> #include "sysemu/block-backend.h"
>>>>>>>>>>> #include "qemu/cutils.h"
>>>>>>>>>>> +#include "qemu/error-report.h"
>>>>>>>>>>> BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>>>>> BlockDriverState *bs,
>>>>>>>>>>> Error **errp)
>>>>>>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function
>>>>>>>>>>> func_fprintf, void *f,
>>>>>>>>>>> if (info->has_format_specific) {
>>>>>>>>>>> func_fprintf(f, "Format specific information:\n");
>>>>>>>>>>> + if (info->format_specific &&
>>>>>>>>>>> + info->format_specific->type ==
>>>>>>>>>>> IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>>>>>>>> + info->format_specific->u.qcow2.data->has_bitmaps ==
>>>>>>>>>>> false) {
>>>>>>>>>>> + warn_report("Failed to load bitmap list");
>>>>>>>>>>> + }
>>>>>>>>>>> bdrv_image_info_specific_dump(func_fprintf, f,
>>>>>>>>>>> info->format_specific);
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>>>>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>>>>>>>> warning?
>>>>>>>>>>
>>>>>>>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>>>>>>>
>>>>>>>>> Dear Kevin,
>>>>>>>>> The reason behind the idea to move the warning to qapi is that we
>>>>>>>>> wouldn't like to print that in case of JSON format.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>>>> index 4897aba..07b99ee 100644
>>>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific
>>>>>>>>>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>>> *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>>> .compat = g_strdup("0.10"),
>>>>>>>>>>> .refcount_bits = s->refcount_bits,
>>>>>>>>>>> + .has_bitmaps = true, /* To handle error check
>>>>>>>>>>> properly */
>>>>>>>>>>> + .bitmaps = NULL, /* Unsupported for version
>>>>>>>>>>> 2 */
>>>>>>>>>>
>>>>>>>>>> .has_bitmaps = false would be nicer if the format doesn't even
>>>>>>>>>> support
>>>>>>>>>> bitmaps. You only need this here because you put the warning in the
>>>>>>>>>> wrong place.
>>>>>>>>>>
>>>>>>>>>>> };
>>>>>>>>>>> } else if (s->qcow_version == 3) {
>>>>>>>>>>> + Qcow2BitmapInfoList *bitmaps;
>>>>>>>>>>> + Error *local_err = NULL;
>>>>>>>>>>> +
>>>>>>>>>>> + bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>>>>>>>
>>>>>>>>>> Here you even had a good error message to print...
>>>>>>>>>>
>>>>>>>>>>> *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>>> .compat = g_strdup("1.1"),
>>>>>>>>>>> .lazy_refcounts = s->compatible_features &
>>>>>>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific
>>>>>>>>>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>>> QCOW2_INCOMPAT_CORRUPT,
>>>>>>>>>>> .has_corrupt = true,
>>>>>>>>>>> .refcount_bits = s->refcount_bits,
>>>>>>>>>>> + .has_bitmaps = !local_err,
>>>>>>>>>>> + .bitmaps = bitmaps,
>>>>>>>>>>> };
>>>>>>>>>>> + /*
>>>>>>>>>>> + * If an error occurs in obtaining bitmaps, ignore
>>>>>>>>>>> + * it to show other QCOW2 specific information.
>>>>>>>>>>> + */
>>>>>>>>>>> + error_free(local_err);
>>>>>>>>>>
>>>>>>>>>> ...but you decided to discard it.
>>>>>>>>>>
>>>>>>>>>> How about you do this here:
>>>>>>>>>>
>>>>>>>>>> warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>>>>>>>
>>>>>>>>> In case of JSON format, we can fail here.
>>>>>>>>> It will happen in the current implementation of the new test #239.
>>>>>>>>
>>>>>>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>>>>>>
>>>>>>>> Essentially, this is the same question I asked here:
>>>>>>>>
>>>>>>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>>>>>>>
>>>>>>>>>> Actually, should this really only be a warning, or in fact an error?
>>>>>>>>>> What's the case where we expect that loading the bitmap list can
>>>>>>>>>> fail,
>>>>>>>>>> but the management tool doesn't need to know that and we just want to
>>>>>>>>>> log a message?
>>>>>>>>
>>>>>>>> I don't understand why it's okay not to print bitmaps without making it
>>>>>>>> an error. Do you have a specific use case in mind for this behaviour?
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We just can't print anything here, as this code is executed from qmp
>>>>>>> command.
>>>>>>>
>>>>>>> It was discussed, that we don't want to fail the whole query, if failed
>>>>>>> to
>>>>>>> obtain bitmaps.
>>>>>>
>>>>>> It's obvious that you don't want to fail the query command, but I
>>>>>> haven't found any explanation for _why_ you want this.
>>>>>>
>>>>>> The thing that is closest to an explanation is "it may be sad to fail
>>>>>> get any information because of repeating some disk/qcow2 error", written
>>>>>> by you in the v4 thread.
>>>>>>
>>>>>> I don't think "may be sad" is a good justification for non-standard
>>>>>> behaviour. If we can't load the bitmaps, the image is broken. And if the
>>>>>> image is broken, the rest of the information may be invalid, too.
>>>>>
>>>>> So, you mean that we go wrong way. And it was my idea. That is sad too.
>>>>>
>>>>> Than, seems like we should add errp to the function and to the full stack
>>>>> down to qmp_query_block. And drop extra partial-success qapi logic.
>>>>
>>>> I'm not necessarily saying that it's the wrong way, though possibly it
>>>> is wrong indeed.
>>>>
>>>> But ignoring some kind of failures is special, so what I was looking for
>>>> were comments or documentation to explain the reason behind it at least.
>>>> Now from the replies I got so far it looks to me that possibly the
>>>> reasons are not only undocumented, but we might actually not have any.
>>>>
>>>>>>> So, to show, that there were error during bitmaps querying
>>>>>>> we decided to use the following semantics:
>>>>>>>
>>>>>>> empty list (has_bitmaps=true) means that there no bitmaps, and there
>>>>>>> were
>>>>>>> no errors during bitmaps quering (if it was)
>>>>>>>
>>>>>>> absence of the field (has_bitmaps=false) means that there _were_ errors
>>>>>>> during
>>>>>>> bitmaps querying.
>>>>>>
>>>>>> ...or that you're running an old QEMU version. I'm not sure if making
>>>>>> old QEMU versions and image errors look the same is a good move.
>>>>>
>>>>> No, for old versions we return empty list, to show that there were no
>>>>> errors.
>>>>
>>>> You mean old image format versions?
>>>
>>> yes.
>>>
>>> I'm talking about old QEMU versions
>>>> that don't even know the 'bitmaps' field.
>>>
>>> hmm. Yes, that's a problem, which we didn't considered.
>>>
>>>>
>>>> A QMP client would have to parse the schema to understand whether a
>>>> missing 'bitmaps' field means that it's talking to an old QEMU (then
>>>> 'bitmaps' doesn't exist in the schema), or that an error happened (then
>>>> the field does exist in the schema).
>>>>
>>>> Looking at the schema is not impossible, so if we require this for a
>>>> good reason, it's okay. But it's not trivial either. If we really want
>>>> to go with this semantics, we should probably mention both the old and
>>>> the new meaning in the QAPI documentation, with the recommendation that
>>>> you parse the schema to determine the meaning of a missing 'bitmaps'
>>>> field.
>>>
>>> Hm, now I think that if we really face the case when we need partial success
>>> of info querying, it should be additional optional parameter which enables
>>> it, like
>>> skip-failed=true (default false) or something, which is more clear than
>>> parsing the
>>> schema. And, which we can add later if needed.
>>
>> Note also, that we already skip some errors, for example,
>> block_crypto_get_specific_info_luks
>> will return NULL on errors (unlike qcow2_get_specific_info, which aborts),
>> and
>> bdrv_query_image_info skip these errors, in same manner as for formats which
>> don't support
>> bdrv_get_specific_info.
>>
>> Looks like a good moment to fix this too, do you agree?
>
> Yes, once we add an Error **errp to .bdrv_get_specific_info, returning
> errors for failing qcrypto_block_get_info() calls as well (both in
> crypto and qcow2) makes sense to me.
>
> Or in fact, remove the errp parameter from qcrypto_block_get_info(). It
> seems to never return errors.
>
> Kevin
>
Dear Kevin,
Thank you very much for your collaboration.
Your suggestion makes the code change even simpler.
I have modified the patches.
Please review a new version #10.
--
With the best regards,
Andrey Shinkevich
