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

[Kevin Wolf <kwolf@redhat.com>]

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

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.

Kevin