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