Re: [Qemu-block] [Qemu-devel] [RFC PATCH] block: Clean up bdrv_img_create()'s error reporting

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 16.10.2018 um 08:41 hat Markus Armbruster geschrieben:
>> bdrv_img_create() takes an Error ** argument and used it in the
>> conventional way, except for one place: when qemu_opts_do_parse()
>> fails, it first reports its error to stderr or the HMP monitor with
>> error_report_err(), then error_setg()'s a generic error.  When the
>> caller reports that second error similarly, this produces two
>> consecutive error messages on stderr or the HMP monitor.  When the
>> caller does something else with it, such as send it via QMP, the first
>> error still goes to stderr or the HMP monitor.  Not good.
>> 
>> Turn the first message into a prefix for the second.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> 
>> This is RFC because I didn't check whether "caller does something else
>> with it" can actually happen with current code, and I'm not sure the
>> prefix is wanted.  I hope we'll answer both questions during review.
>
> The only caller that passes non-NULL options and can therefore even run
> into this error path is qemu-img.c. It passes any Error it receives to
> error_reportf_err().
>
> Current behaviour:
>
>     $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
>     qemu-img: Invalid parameter 'foo'
>     qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2'
>
> Assumed behaviour with your patch (can't test because I don't have
> error_propagate_prepend() yet and its addition is not a separate patch):

Sorry, forgot
Based-on: <address@hidden>

Don't bother testing, my patch is buggy.

>     $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
>     qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2': 
> Invalid parameter 'foo'
>
> Combining two redundant messages into a single line is nice. Keeping
> the redundant information in it ('invalid options'/'invalid parameter')
> isn't perfect, though.
>
> If instead of prepending, we just propagate the error, would this
> actually lack any important information?
>
>     $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
>     qemu-img: /tmp/test.qcow2: Invalid parameter 'foo'

Works for me.  Thanks!