Re: [Qemu-devel] [PATCH v4] qemu-img info lists bitmap directory entries

[Markus Armbruster]

Please don't use "Content-Transfer-Encoding: base64".

Andrey Shinkevich <address@hidden> writes:

> On 07.12.2018 19:20, Eric Blake wrote:
>
>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
[...]
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific 
>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>               .refcount_bits      = s->refcount_bits,
>>>           };
>>>       } else if (s->qcow_version == 3) {
>>> +        Qcow2BitmapInfoList *bitmaps;
>>> +        Error *local_err = NULL;
>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> +        if (local_err != NULL) {
>>> +            error_report_err(local_err);
>>> +        }
>>
>> Ouch. Calling error_report_err() doesn't always work in QMP context; 
>> better would be to plumb Error **errp back up to the caller, if 
>> getting this specific information can fail and we want the overall 
>> query-block to fail.  Or, we could decide that failure to get bitmap 
>> info is non-fatal, and that it was just a best-effort attempt to get 
>> more info, where we then ignore the failure, rather than trying to 
>> report it incorrectly.
> In my test environment, error_report_err() and warn_report_err() both 
> cause printing an error message as the only output of 'qemu-img info' 
> command.

Eric is right, using error_report() & friends in QMP context is almost
always wrong.  Here's the issue in a bit more detail.  Consider QMP
command query-block.  Reverse call stack:

    qmp_query_block()
    bdrv_query_info()
    bdrv_block_device_info()
    bdrv_query_image_info()
    bdrv_get_specific_info()
    qcow2_get_specific_info()

The error you report in qcow2_get_specific_info() gets reported to
stderr.  It is *not* reported to the QMP client.  Why is that a good
idea?

[...]