| Message ID | 402b5ae030b5d15cd5b612493de366789c73183a.1389747507.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
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; >
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 >
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 >>
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.
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;
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;
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(-)