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 <arm...@redhat.com>
> ---
>
> 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
