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

[Kevin Wolf]

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):

    $ ./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'

Kevin