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

[Kevin Wolf]

Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
> > 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.
> 
> Note also, that we already skip some errors, for example, 
> block_crypto_get_specific_info_luks
> will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and
> bdrv_query_image_info skip these errors, in same manner as for formats which 
> don't support
> bdrv_get_specific_info.
> 
> Looks like a good moment to fix this too, do you agree?

Yes, once we add an Error **errp to .bdrv_get_specific_info, returning
errors for failing qcrypto_block_get_info() calls as well (both in
crypto and qcow2) makes sense to me.

Or in fact, remove the errp parameter from qcrypto_block_get_info(). It
seems to never return errors.

Kevin