[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] block: Clean up bdrv_img_create()'s error r
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC PATCH] block: Clean up bdrv_img_create()'s error reporting |
Date: |
Tue, 16 Oct 2018 17:40:02 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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!