Before the from qerror_report() to error_setg(), hints looked like
this:

    qerror_report(QERR_MACRO, ... arguments ...);
    error_printf_unless_qmp(... hint ...);

error_printf_unless_qmp() made perfect sense: it printed exactly when
qerror_report() did.

After the conversion to error_setg():

    error_setg(errp, QERR_MACRO, ... arguments ...);
    error_printf_unless_qmp(... hint ...);

The "unless QMP part" still made some sense; in QMP context, the
caller generally uses the error as QMP response instead of printing
it.

However, everything else is wrong.  If the caller handles the error,
the hint gets printed anyway (unless QMP).  If the caller reports the
error, the hint gets printed *before* the report (unless QMP) or not
at all (if QMP).

Commit 50b7b000c91 fixed this by making hints a member of Error.  It
kept printing hints with error_printf_unless_qmp():

     void error_report_err(Error *err)
     {
	 error_report("%s", error_get_pretty(err));
    +    if (err->hint) {
    +        error_printf_unless_qmp("%s\n", err->hint->str);
    +    }
	 error_free(err);
     }

This is wrong.  We should (and now can) print the hint exactly when we
print the error.

The mistake has since been copied to warn_report_err().

Fix both to use error_printf().

Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/error.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/error.c b/util/error.c
index 934a78e1b1..712b4d4b5d 100644
--- a/util/error.c
+++ b/util/error.c
@@ -223,7 +223,7 @@ void error_report_err(Error *err)
 {
     error_report("%s", error_get_pretty(err));
     if (err->hint) {
-        error_printf_unless_qmp("%s", err->hint->str);
+        error_printf("%s", err->hint->str);
     }
     error_free(err);
 }
@@ -232,7 +232,7 @@ void warn_report_err(Error *err)
 {
     warn_report("%s", error_get_pretty(err));
     if (err->hint) {
-        error_printf_unless_qmp("%s", err->hint->str);
+        error_printf("%s", err->hint->str);
     }
     error_free(err);
 }
-- 
2.17.2

On 4/16/19 10:38 AM, Markus Armbruster wrote:
> Before the from qerror_report() to error_setg(), hints looked like

s/the from/the change from/

> this:
> 
>     qerror_report(QERR_MACRO, ... arguments ...);
>     error_printf_unless_qmp(... hint ...);
> 
> error_printf_unless_qmp() made perfect sense: it printed exactly when
> qerror_report() did.
> 
> After the conversion to error_setg():

Commit id?

> 
>     error_setg(errp, QERR_MACRO, ... arguments ...);
>     error_printf_unless_qmp(... hint ...);
> 
> The "unless QMP part" still made some sense; in QMP context, the
> caller generally uses the error as QMP response instead of printing
> it.
> 
> However, everything else is wrong.  If the caller handles the error,
> the hint gets printed anyway (unless QMP).  If the caller reports the
> error, the hint gets printed *before* the report (unless QMP) or not
> at all (if QMP).
> 
> Commit 50b7b000c91 fixed this by making hints a member of Error.  It

Belongs to 2.5.0.

> kept printing hints with error_printf_unless_qmp():
> 
>      void error_report_err(Error *err)
>      {
> 	 error_report("%s", error_get_pretty(err));
>     +    if (err->hint) {
>     +        error_printf_unless_qmp("%s\n", err->hint->str);
>     +    }
> 	 error_free(err);
>      }
> 
> This is wrong.  We should (and now can) print the hint exactly when we
> print the error.
> 
> The mistake has since been copied to warn_report_err().

Commit id?

> 
> Fix both to use error_printf().
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/error.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Probably too late for 4.0-rc4. Then again, it regressed in 2.5, so
having yet one more release with the lame output doesn't hurt.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 4/16/19 10:38 AM, Markus Armbruster wrote:
>> Before the from qerror_report() to error_setg(), hints looked like
>
> s/the from/the change from/
>
>> this:
>> 
>>     qerror_report(QERR_MACRO, ... arguments ...);
>>     error_printf_unless_qmp(... hint ...);
>> 
>> error_printf_unless_qmp() made perfect sense: it printed exactly when
>> qerror_report() did.
>> 
>> After the conversion to error_setg():
>
> Commit id?

Many.  From the cover letter of the series that finally killed
qerror_report: "After a bit over a year and many patches, QError is
finally ripe."

Here are two known to have messed up hints (commit 50b7b000c91 says so):

7216ae3d1a11e07192623ad04d450e98bf1f3d10
d282842999b914c38c8be4659012aa619c22af8b

>> 
>>     error_setg(errp, QERR_MACRO, ... arguments ...);
>>     error_printf_unless_qmp(... hint ...);
>> 
>> The "unless QMP part" still made some sense; in QMP context, the
>> caller generally uses the error as QMP response instead of printing
>> it.
>> 
>> However, everything else is wrong.  If the caller handles the error,
>> the hint gets printed anyway (unless QMP).  If the caller reports the
>> error, the hint gets printed *before* the report (unless QMP) or not
>> at all (if QMP).
>> 
>> Commit 50b7b000c91 fixed this by making hints a member of Error.  It
>
> Belongs to 2.5.0.
>
>> kept printing hints with error_printf_unless_qmp():
>> 
>>      void error_report_err(Error *err)
>>      {
>> 	 error_report("%s", error_get_pretty(err));
>>     +    if (err->hint) {
>>     +        error_printf_unless_qmp("%s\n", err->hint->str);
>>     +    }
>> 	 error_free(err);
>>      }
>> 
>> This is wrong.  We should (and now can) print the hint exactly when we
>> print the error.
>> 
>> The mistake has since been copied to warn_report_err().
>
> Commit id?

e43ead1d0b9c467af4a2af8f5177316a0ccb5602

>> 
>> Fix both to use error_printf().
>> 
>> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/error.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>
> Probably too late for 4.0-rc4. Then again, it regressed in 2.5, so
> having yet one more release with the lame output doesn't hurt.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Markus Armbruster <armbru@redhat.com> writes:

> Before the from qerror_report() to error_setg(), hints looked like
> this:
>
>     qerror_report(QERR_MACRO, ... arguments ...);
>     error_printf_unless_qmp(... hint ...);
>
> error_printf_unless_qmp() made perfect sense: it printed exactly when
> qerror_report() did.
>
> After the conversion to error_setg():
>
>     error_setg(errp, QERR_MACRO, ... arguments ...);
>     error_printf_unless_qmp(... hint ...);
>
> The "unless QMP part" still made some sense; in QMP context, the
> caller generally uses the error as QMP response instead of printing
> it.
>
> However, everything else is wrong.  If the caller handles the error,
> the hint gets printed anyway (unless QMP).  If the caller reports the
> error, the hint gets printed *before* the report (unless QMP) or not
> at all (if QMP).
>
> Commit 50b7b000c91 fixed this by making hints a member of Error.  It
> kept printing hints with error_printf_unless_qmp():
>
>      void error_report_err(Error *err)
>      {
> 	 error_report("%s", error_get_pretty(err));
>     +    if (err->hint) {
>     +        error_printf_unless_qmp("%s\n", err->hint->str);
>     +    }
> 	 error_free(err);
>      }
>
> This is wrong.  We should (and now can) print the hint exactly when we
> print the error.
>
> The mistake has since been copied to warn_report_err().
>
> Fix both to use error_printf().
>
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Queued.

16.04.2019 18:38, Markus Armbruster wrote:
> Before the from qerror_report() to error_setg(), hints looked like
> this:
> 
>      qerror_report(QERR_MACRO, ... arguments ...);
>      error_printf_unless_qmp(... hint ...);
> 
> error_printf_unless_qmp() made perfect sense: it printed exactly when
> qerror_report() did.
> 
> After the conversion to error_setg():
> 
>      error_setg(errp, QERR_MACRO, ... arguments ...);
>      error_printf_unless_qmp(... hint ...);
> 
> The "unless QMP part" still made some sense; in QMP context, the
> caller generally uses the error as QMP response instead of printing
> it.
> 
> However, everything else is wrong.  If the caller handles the error,
> the hint gets printed anyway (unless QMP).  If the caller reports the
> error, the hint gets printed *before* the report (unless QMP) or not
> at all (if QMP).
> 
> Commit 50b7b000c91 fixed this by making hints a member of Error.  It
> kept printing hints with error_printf_unless_qmp():
> 
>       void error_report_err(Error *err)
>       {
> 	 error_report("%s", error_get_pretty(err));
>      +    if (err->hint) {
>      +        error_printf_unless_qmp("%s\n", err->hint->str);
>      +    }
> 	 error_free(err);
>       }
> 
> This is wrong.  We should (and now can) print the hint exactly when we
> print the error.
> 
> The mistake has since been copied to warn_report_err().
> 
> Fix both to use error_printf().
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/error.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util/error.c b/util/error.c
> index 934a78e1b1..712b4d4b5d 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -223,7 +223,7 @@ void error_report_err(Error *err)
>   {
>       error_report("%s", error_get_pretty(err));
>       if (err->hint) {
> -        error_printf_unless_qmp("%s", err->hint->str);
> +        error_printf("%s", err->hint->str);
>       }
>       error_free(err);
>   }
> @@ -232,7 +232,7 @@ void warn_report_err(Error *err)
>   {
>       warn_report("%s", error_get_pretty(err));
>       if (err->hint) {
> -        error_printf_unless_qmp("%s", err->hint->str);
> +        error_printf("%s", err->hint->str);
>       }
>       error_free(err);
>   }
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir