[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:35:57 +0000 |
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.
--
Best regards,
Vladimir
- Re: [Qemu-devel] [PATCH v9 1/2] qemu-img info lists bitmap directory entries, (continued)
- 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, 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, 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