qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v9 1/2] qemu-img info lists bitmap directory ent


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v9 1/2] qemu-img info lists bitmap directory entries
Date: Tue, 29 Jan 2019 12:50:07 +0000

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. 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.

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.

-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]