[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 12:57:16 +0000 |
29.01.2019 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 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.
Hmm, or we can?
Ok, for json output, it may be ok to print warnings to stderr, while formatted
json to stdout.
And for qapi, warning will go to logs, which is not bad too.
Ok, now I don't see any counterarguments..
--
Best regards,
Vladimir
- [Qemu-devel] [PATCH v9 0/2] qemu-img info lists bitmap directory entries, Andrey Shinkevich, 2019/01/28
- [Qemu-devel] [PATCH v9 2/2] qemu-img info: bitmaps extension new test 239, Andrey Shinkevich, 2019/01/28
- [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 <=
- 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, 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