[Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Peter Crosthwaite]

Use fprintf(stderr instead. This removes dependency of libqemuutil.a
on the monitor.

We can further justify this change, in that this code path should only
trigger under a fatal error condition. fprintf-stderr is probably the
appropriate medium as under a fatal error conidition the monitor itself
may be down and out for the count. So assertion failure messages should
go lowest common denominator - straight to stderr.

Fixes the build as reported by Kevin Wolf. Issue debugged and change
suggested by Luiz Capitulino. Issue introduced by
5d24ee70bcbcf578614193526bcd5ed30a8eb16c.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 util/error.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/error.c b/util/error.c
index f11f1d5..7c7650c 100644
--- a/util/error.c
+++ b/util/error.c
@@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        fprintf(stderr, "%s", error_get_pretty(err));
         abort();
     }
 
@@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        fprintf(stderr, "%s", error_get_pretty(err));
         abort();
     }
 
@@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        fprintf(stderr, "%s", error_get_pretty(err));
         abort();
     }
 
@@ -171,7 +171,7 @@ void error_free(Error *err)
 void error_propagate(Error **dst_err, Error *local_err)
 {
     if (local_err && dst_err == &error_abort) {
-        error_report("%s", error_get_pretty(local_err));
+        fprintf(stderr, "%s", error_get_pretty(local_err));
         abort();
     } else if (dst_err && !*dst_err) {
         *dst_err = local_err;
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Andreas Färber]

Am 15.01.2014 03:29, schrieb Peter Crosthwaite:
> Use fprintf(stderr instead. This removes dependency of libqemuutil.a
> on the monitor.
> 
> We can further justify this change, in that this code path should only
> trigger under a fatal error condition. fprintf-stderr is probably the
> appropriate medium as under a fatal error conidition the monitor itself
> may be down and out for the count. So assertion failure messages should
> go lowest common denominator - straight to stderr.

Actually I thought the point of error_report() was to add an appropriate
prefix "qemu-system-foo: ..." to the error message and an optional
timestamp, not to send it to the monitor...

> 
> Fixes the build as reported by Kevin Wolf. Issue debugged and change
> suggested by Luiz Capitulino. Issue introduced by
> 5d24ee70bcbcf578614193526bcd5ed30a8eb16c.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  util/error.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/util/error.c b/util/error.c
> index f11f1d5..7c7650c 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));

You need to add \n if you do this.

Andreas

>          abort();
>      }
>  
> @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));
>          abort();
>      }
>  
> @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));
>          abort();
>      }
>  
> @@ -171,7 +171,7 @@ void error_free(Error *err)
>  void error_propagate(Error **dst_err, Error *local_err)
>  {
>      if (local_err && dst_err == &error_abort) {
> -        error_report("%s", error_get_pretty(local_err));
> +        fprintf(stderr, "%s", error_get_pretty(local_err));
>          abort();
>      } else if (dst_err && !*dst_err) {
>          *dst_err = local_err;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Peter Crosthwaite]

On Wed, Jan 15, 2014 at 12:55 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 15.01.2014 03:29, schrieb Peter Crosthwaite:
>> Use fprintf(stderr instead. This removes dependency of libqemuutil.a
>> on the monitor.
>>
>> We can further justify this change, in that this code path should only
>> trigger under a fatal error condition. fprintf-stderr is probably the
>> appropriate medium as under a fatal error conidition the monitor itself
>> may be down and out for the count. So assertion failure messages should
>> go lowest common denominator - straight to stderr.
>
> Actually I thought the point of error_report() was to add an appropriate
> prefix "qemu-system-foo: ..." to the error message and an optional
> timestamp, not to send it to the monitor...
>

Well this patch routes it away from the monitor. Never implemented
prefixing though. My main motivations were:

1: Text reduction. No need to define local_err and assert them constantly.
2: Full backtraceability of the error.

Regards,
Peter

>>
>> Fixes the build as reported by Kevin Wolf. Issue debugged and change
>> suggested by Luiz Capitulino. Issue introduced by
>> 5d24ee70bcbcf578614193526bcd5ed30a8eb16c.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  util/error.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/util/error.c b/util/error.c
>> index f11f1d5..7c7650c 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>      err->err_class = err_class;
>>
>>      if (errp == &error_abort) {
>> -        error_report("%s", error_get_pretty(err));
>> +        fprintf(stderr, "%s", error_get_pretty(err));
>
> You need to add \n if you do this.
>
> Andreas
>
>>          abort();
>>      }
>>
>> @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>>      err->err_class = err_class;
>>
>>      if (errp == &error_abort) {
>> -        error_report("%s", error_get_pretty(err));
>> +        fprintf(stderr, "%s", error_get_pretty(err));
>>          abort();
>>      }
>>
>> @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
>>      err->err_class = err_class;
>>
>>      if (errp == &error_abort) {
>> -        error_report("%s", error_get_pretty(err));
>> +        fprintf(stderr, "%s", error_get_pretty(err));
>>          abort();
>>      }
>>
>> @@ -171,7 +171,7 @@ void error_free(Error *err)
>>  void error_propagate(Error **dst_err, Error *local_err)
>>  {
>>      if (local_err && dst_err == &error_abort) {
>> -        error_report("%s", error_get_pretty(local_err));
>> +        fprintf(stderr, "%s", error_get_pretty(local_err));
>>          abort();
>>      } else if (dst_err && !*dst_err) {
>>          *dst_err = local_err;
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Peter Crosthwaite]

On Wed, Jan 15, 2014 at 1:31 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, Jan 15, 2014 at 12:55 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 15.01.2014 03:29, schrieb Peter Crosthwaite:
>>> Use fprintf(stderr instead. This removes dependency of libqemuutil.a
>>> on the monitor.
>>>
>>> We can further justify this change, in that this code path should only
>>> trigger under a fatal error condition. fprintf-stderr is probably the
>>> appropriate medium as under a fatal error conidition the monitor itself
>>> may be down and out for the count. So assertion failure messages should
>>> go lowest common denominator - straight to stderr.
>>
>> Actually I thought the point of error_report() was to add an appropriate
>> prefix "qemu-system-foo: ..." to the error message and an optional
>> timestamp, not to send it to the monitor...
>>
>
> Well this patch routes it away from the monitor. Never implemented
> prefixing though. My main motivations were:
>
> 1: Text reduction. No need to define local_err and assert them constantly.
> 2: Full backtraceability of the error.
>

Sry misread your mail, you were commenting error_report not error_abort as
I read it if your wondering why my mail makes no sense at all. Sry for the
noise :S

> Regards,
> Peter
>
>>>
>>> Fixes the build as reported by Kevin Wolf. Issue debugged and change
>>> suggested by Luiz Capitulino. Issue introduced by
>>> 5d24ee70bcbcf578614193526bcd5ed30a8eb16c.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>>  util/error.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/util/error.c b/util/error.c
>>> index f11f1d5..7c7650c 100644
>>> --- a/util/error.c
>>> +++ b/util/error.c
>>> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>>      err->err_class = err_class;
>>>
>>>      if (errp == &error_abort) {
>>> -        error_report("%s", error_get_pretty(err));
>>> +        fprintf(stderr, "%s", error_get_pretty(err));
>>
>> You need to add \n if you do this.
>>

Fixed. V2 en-route.

Regards,
Peter

>> Andreas
>>
>>>          abort();
>>>      }
>>>
>>> @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>>>      err->err_class = err_class;
>>>
>>>      if (errp == &error_abort) {
>>> -        error_report("%s", error_get_pretty(err));
>>> +        fprintf(stderr, "%s", error_get_pretty(err));
>>>          abort();
>>>      }
>>>
>>> @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
>>>      err->err_class = err_class;
>>>
>>>      if (errp == &error_abort) {
>>> -        error_report("%s", error_get_pretty(err));
>>> +        fprintf(stderr, "%s", error_get_pretty(err));
>>>          abort();
>>>      }
>>>
>>> @@ -171,7 +171,7 @@ void error_free(Error *err)
>>>  void error_propagate(Error **dst_err, Error *local_err)
>>>  {
>>>      if (local_err && dst_err == &error_abort) {
>>> -        error_report("%s", error_get_pretty(local_err));
>>> +        fprintf(stderr, "%s", error_get_pretty(local_err));
>>>          abort();
>>>      } else if (dst_err && !*dst_err) {
>>>          *dst_err = local_err;
>>>
>>
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>

Re: [Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 15.01.2014 03:29, schrieb Peter Crosthwaite:
>> Use fprintf(stderr instead. This removes dependency of libqemuutil.a
>> on the monitor.
>> 
>> We can further justify this change, in that this code path should only
>> trigger under a fatal error condition. fprintf-stderr is probably the
>> appropriate medium as under a fatal error conidition the monitor itself
>> may be down and out for the count. So assertion failure messages should
>> go lowest common denominator - straight to stderr.
>
> Actually I thought the point of error_report() was to add an appropriate
> prefix "qemu-system-foo: ..." to the error message and an optional
> timestamp, not to send it to the monitor...

Exactly.  Unwanted loss of functionality.

Please try adding stubs/mon-set-error.o to whatever is missing cur_mon
instead.

>> Fixes the build as reported by Kevin Wolf. Issue debugged and change
>> suggested by Luiz Capitulino. Issue introduced by
>> 5d24ee70bcbcf578614193526bcd5ed30a8eb16c.
>> 
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> 
>>  util/error.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/util/error.c b/util/error.c
>> index f11f1d5..7c7650c 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>      err->err_class = err_class;
>>  
>>      if (errp == &error_abort) {
>> -        error_report("%s", error_get_pretty(err));
>> +        fprintf(stderr, "%s", error_get_pretty(err));
>
> You need to add \n if you do this.

Yup.

Re: [Qemu-devel] [PATCH build-fix v1 1/1] error: Don't use error_report() for assertion msgs.

[Luiz Capitulino]

On Tue, 14 Jan 2014 18:29:50 -0800
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Use fprintf(stderr instead. This removes dependency of libqemuutil.a
> on the monitor.
> 
> We can further justify this change, in that this code path should only
> trigger under a fatal error condition. fprintf-stderr is probably the
> appropriate medium as under a fatal error conidition the monitor itself
> may be down and out for the count. So assertion failure messages should
> go lowest common denominator - straight to stderr.
> 
> Fixes the build as reported by Kevin Wolf. Issue debugged and change
> suggested by Luiz Capitulino. Issue introduced by
> 5d24ee70bcbcf578614193526bcd5ed30a8eb16c.

Thanks for doing this Peter! I missed the prefix feature (well pointed
by Markus), but at least we unblocked other people's testing and can
fix it w/o rushing.

> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  util/error.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/util/error.c b/util/error.c
> index f11f1d5..7c7650c 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));
>          abort();
>      }
>  
> @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));
>          abort();
>      }
>  
> @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
>      err->err_class = err_class;
>  
>      if (errp == &error_abort) {
> -        error_report("%s", error_get_pretty(err));
> +        fprintf(stderr, "%s", error_get_pretty(err));
>          abort();
>      }
>  
> @@ -171,7 +171,7 @@ void error_free(Error *err)
>  void error_propagate(Error **dst_err, Error *local_err)
>  {
>      if (local_err && dst_err == &error_abort) {
> -        error_report("%s", error_get_pretty(local_err));
> +        fprintf(stderr, "%s", error_get_pretty(local_err));
>          abort();
>      } else if (dst_err && !*dst_err) {
>          *dst_err = local_err;