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 <armbru@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.

 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 5d51419d21..08aafc20a2 100644
--- a/block.c
+++ b/block.c
@@ -4803,9 +4803,9 @@ void bdrv_img_create(const char *filename, const char *fmt,
     if (options) {
         qemu_opts_do_parse(opts, options, NULL, &local_err);
         if (local_err) {
-            error_report_err(local_err);
-            local_err = NULL;
-            error_setg(errp, "Invalid options for file format '%s'", fmt);
+            error_propagate_prepend(errp, local_err,
+                                    "Invalid options for file format '%s'",
+                                    fmt);
             goto out;
         }
     }
-- 
2.17.1

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 <armbru@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

Kevin Wolf <kwolf@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 <armbru@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-armbru@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!