[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory ent
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries |
Date: |
Tue, 29 Jan 2019 14:06:30 +0000 |
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.
>
>> 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.
>
> Almost worse than making the QAPI interface confusing, however, is that
> we throw away the real error message without giving it to the user.
>
>> So qapi user, as well as json-output user should follow this semantics in
>> understanding the output. And as well as for qapi, it's incorrect to add text
>> messages to json output.
>>
>> On the other hand, it seems reasonable to put additional notice for
>> human-format
>> user.
>
> Even for QMP, a log message on stderr is better than nothing.
>
Agreed.
--
Best regards,
Vladimir
- Re: [Qemu-devel] [PATCH v9 2/2] qemu-img info: bitmaps extension new test 239, (continued)
- [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Andrey Shinkevich, 2019/01/28
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Eric Blake, 2019/01/28
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Kevin Wolf, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Andrey Shinkevich, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Kevin Wolf, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Vladimir Sementsov-Ogievskiy, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Vladimir Sementsov-Ogievskiy, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Kevin Wolf, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries,
Vladimir Sementsov-Ogievskiy <=
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Kevin Wolf, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Vladimir Sementsov-Ogievskiy, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Vladimir Sementsov-Ogievskiy, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Kevin Wolf, 2019/01/29
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, Andrey Shinkevich, 2019/01/30