Kevin Wolf <kw...@redhat.com> 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 <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):
Sorry, forgot
Based-on: <20181015115309.17089-1-arm...@redhat.com>
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!
