| Message ID | 20140114132248.3f0be1dd@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 15, 2014 at 4:22 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Tue, 14 Jan 2014 17:44:51 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 14.01.2014 um 04:38 hat Edgar E. Iglesias geschrieben: >> > On Tue, Jan 14, 2014 at 09:27:10AM +1000, Peter Crosthwaite wrote: >> > > Ping, >> > > >> > > Has this one been forgotten or are there issues? PMM had a small >> > > comment, but he waived it AFAICT. >> > >> > Pong, >> > >> > I've merged it now, thanks! >> >> I believe it's something in this pull requests that breaks make check. > > And you're right. But first, let me confirm that we're talking about the > same breakage. This is what I'm getting: > > make tests/check-qom-interface > libqemuutil.a(qemu-error.o): In function `error_vprintf': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:23: undefined reference to `cur_mon' > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `cur_mon' > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `monitor_vprintf' > libqemuutil.a(qemu-error.o): In function `error_printf_unless_qmp': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:47: undefined reference to `monitor_cur_is_qmp' > libqemuutil.a(qemu-error.o): In function `error_print_loc': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:174: undefined reference to `cur_mon' > collect2: error: ld returned 1 exit status > make: *** [tests/check-qom-interface] Error 1 > > I tried bisecting it, but git bisect weren't capable of finding the > culprit. So debugged it, and the problem was introduced by: > > commit 594278718323ca7bffaab0fb7fc6c82fa2c1cd5f > Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Date: Wed Jan 1 18:49:52 2014 -0800 > > qerror: Remove assert_no_error() > > There isn't nothing really wrong with this commit. The problem seems to > be that the tests link against libqemuutil.a and this library pulls in > everything from util/. The commit above changed util/error.c to call > error_report(), which depends on 'cur_mon', which is only made available > by monitor.o. > > I don't think we want to mess up with including monitor.o on libqemuutil.a. > Besides, I now find it a bit weird to call error_report() from an error > reporting function. So it's better to just call fprintf(stderr,) instead. > > Peter, Markus, are you ok with this patch? Patch is good. Acked-by: Peter Crosthwiate <peter.crosthwaite@xilinx.com> > > PS: I do run make check before sending a pull request, and did run this > time too. Not sure how I didn't catch this. Thanks for the report > Kevin! > I ran make check before sending out the patches too. Not sure what happened since. Regards, Peter > 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; >
On Wed, Jan 15, 2014 at 8:26 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Jan 15, 2014 at 4:22 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote: >> On Tue, 14 Jan 2014 17:44:51 +0100 >> Kevin Wolf <kwolf@redhat.com> wrote: >> >>> Am 14.01.2014 um 04:38 hat Edgar E. Iglesias geschrieben: >>> > On Tue, Jan 14, 2014 at 09:27:10AM +1000, Peter Crosthwaite wrote: >>> > > Ping, >>> > > >>> > > Has this one been forgotten or are there issues? PMM had a small >>> > > comment, but he waived it AFAICT. >>> > >>> > Pong, >>> > >>> > I've merged it now, thanks! >>> >>> I believe it's something in this pull requests that breaks make check. >> >> And you're right. But first, let me confirm that we're talking about the >> same breakage. This is what I'm getting: >> >> make tests/check-qom-interface >> libqemuutil.a(qemu-error.o): In function `error_vprintf': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:23: undefined reference to `cur_mon' >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `cur_mon' >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `monitor_vprintf' >> libqemuutil.a(qemu-error.o): In function `error_printf_unless_qmp': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:47: undefined reference to `monitor_cur_is_qmp' >> libqemuutil.a(qemu-error.o): In function `error_print_loc': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:174: undefined reference to `cur_mon' >> collect2: error: ld returned 1 exit status >> make: *** [tests/check-qom-interface] Error 1 >> >> I tried bisecting it, but git bisect weren't capable of finding the >> culprit. So debugged it, and the problem was introduced by: >> >> commit 594278718323ca7bffaab0fb7fc6c82fa2c1cd5f >> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Date: Wed Jan 1 18:49:52 2014 -0800 >> >> qerror: Remove assert_no_error() >> >> There isn't nothing really wrong with this commit. The problem seems to >> be that the tests link against libqemuutil.a and this library pulls in >> everything from util/. The commit above changed util/error.c to call >> error_report(), which depends on 'cur_mon', which is only made available >> by monitor.o. >> >> I don't think we want to mess up with including monitor.o on libqemuutil.a. >> Besides, I now find it a bit weird to call error_report() from an error >> reporting function. So it's better to just call fprintf(stderr,) instead. >> >> Peter, Markus, are you ok with this patch? > > Patch is good. > > Acked-by: Peter Crosthwiate <peter.crosthwaite@xilinx.com> > Do you want to spin this as a patch or should I? >> >> PS: I do run make check before sending a pull request, and did run this >> time too. Not sure how I didn't catch this. Thanks for the report >> Kevin! >> > > I ran make check before sending out the patches too. Not sure what > happened since. > I tested the tip of the merged branch and it is ok: commit c950114286ea358a93ce632db0421945e1008395 Author: Luiz Capitulino <lcapitulino@redhat.com> Date: Sun Dec 29 22:39:58 2013 -0500 migration: qmp_migrate(): keep working after syntax error If a user or QMP client enter a bad syntax for the migrate command in QMP/HMP, then the migrate command will never succeed from that point on. Something merged since has had an effect, so bisection is impossible as we would need to reorder the history to figure out what it was and I don't think it's worth it. Regards, Peter > Regards, > Peter > >> 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; >>
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Tue, 14 Jan 2014 17:44:51 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 14.01.2014 um 04:38 hat Edgar E. Iglesias geschrieben: >> > On Tue, Jan 14, 2014 at 09:27:10AM +1000, Peter Crosthwaite wrote: >> > > Ping, >> > > >> > > Has this one been forgotten or are there issues? PMM had a small >> > > comment, but he waived it AFAICT. >> > >> > Pong, >> > >> > I've merged it now, thanks! >> >> I believe it's something in this pull requests that breaks make check. > > And you're right. But first, let me confirm that we're talking about the > same breakage. This is what I'm getting: > > make tests/check-qom-interface > libqemuutil.a(qemu-error.o): In function `error_vprintf': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:23: undefined reference to `cur_mon' > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `cur_mon' > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `monitor_vprintf' > libqemuutil.a(qemu-error.o): In function `error_printf_unless_qmp': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:47: undefined reference to `monitor_cur_is_qmp' > libqemuutil.a(qemu-error.o): In function `error_print_loc': > /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:174: undefined reference to `cur_mon' > collect2: error: ld returned 1 exit status > make: *** [tests/check-qom-interface] Error 1 > > I tried bisecting it, but git bisect weren't capable of finding the > culprit. So debugged it, and the problem was introduced by: > > commit 594278718323ca7bffaab0fb7fc6c82fa2c1cd5f > Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Date: Wed Jan 1 18:49:52 2014 -0800 > > qerror: Remove assert_no_error() > Are you sure you don't mean commit 5d24ee7 "error: Add error_abort"? > There isn't nothing really wrong with this commit. The problem seems to > be that the tests link against libqemuutil.a and this library pulls in > everything from util/. The commit above changed util/error.c to call > error_report(), Yes, 5d24ee7 does that. > which depends on 'cur_mon', which is only made available > by monitor.o. And stubs/mon-set-error.o > I don't think we want to mess up with including monitor.o on libqemuutil.a. > Besides, I now find it a bit weird to call error_report() from an error > reporting function. So it's better to just call fprintf(stderr,) instead. It's not weird at all. Higher layer calls lower layer. > Peter, Markus, are you ok with this patch? > > PS: I do run make check before sending a pull request, and did run this > time too. Not sure how I didn't catch this. Thanks for the report > Kevin! > > 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; Error message screwed up! Before: $ qemu -msg timestamp=on -global i440FX-pcihost.foo=bar 2014-01-15T09:50:18.066725Z upstream-qemu: Property '.foo' not found Aborted (core dumped) After: Property '.foo' not foundAborted (core dumped) Note the loss of timestamp, name of executable, location, and the final newline. Please fix. Amazing super-secret trick to get error messages fit for human consumption: reproduce them before you commit! ;-P
On Wed, Jan 15, 2014 at 7:55 PM, Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> On Tue, 14 Jan 2014 17:44:51 +0100 >> Kevin Wolf <kwolf@redhat.com> wrote: >> >>> Am 14.01.2014 um 04:38 hat Edgar E. Iglesias geschrieben: >>> > On Tue, Jan 14, 2014 at 09:27:10AM +1000, Peter Crosthwaite wrote: >>> > > Ping, >>> > > >>> > > Has this one been forgotten or are there issues? PMM had a small >>> > > comment, but he waived it AFAICT. >>> > >>> > Pong, >>> > >>> > I've merged it now, thanks! >>> >>> I believe it's something in this pull requests that breaks make check. >> >> And you're right. But first, let me confirm that we're talking about the >> same breakage. This is what I'm getting: >> >> make tests/check-qom-interface >> libqemuutil.a(qemu-error.o): In function `error_vprintf': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:23: undefined reference to `cur_mon' >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `cur_mon' >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `monitor_vprintf' >> libqemuutil.a(qemu-error.o): In function `error_printf_unless_qmp': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:47: undefined reference to `monitor_cur_is_qmp' >> libqemuutil.a(qemu-error.o): In function `error_print_loc': >> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:174: undefined reference to `cur_mon' >> collect2: error: ld returned 1 exit status >> make: *** [tests/check-qom-interface] Error 1 >> >> I tried bisecting it, but git bisect weren't capable of finding the >> culprit. So debugged it, and the problem was introduced by: >> >> commit 594278718323ca7bffaab0fb7fc6c82fa2c1cd5f >> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Date: Wed Jan 1 18:49:52 2014 -0800 >> >> qerror: Remove assert_no_error() >> > > Are you sure you don't mean commit 5d24ee7 "error: Add error_abort"? > >> There isn't nothing really wrong with this commit. The problem seems to >> be that the tests link against libqemuutil.a and this library pulls in >> everything from util/. The commit above changed util/error.c to call >> error_report(), > > Yes, 5d24ee7 does that. > >> which depends on 'cur_mon', which is only made available >> by monitor.o. > > And stubs/mon-set-error.o > >> I don't think we want to mess up with including monitor.o on libqemuutil.a. >> Besides, I now find it a bit weird to call error_report() from an error >> reporting function. So it's better to just call fprintf(stderr,) instead. > > It's not weird at all. Higher layer calls lower layer. > >> Peter, Markus, are you ok with this patch? >> >> PS: I do run make check before sending a pull request, and did run this >> time too. Not sure how I didn't catch this. Thanks for the report >> Kevin! >> >> 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; > > Error message screwed up! > > Before: > > $ qemu -msg timestamp=on -global i440FX-pcihost.foo=bar > 2014-01-15T09:50:18.066725Z upstream-qemu: Property '.foo' not found > Aborted (core dumped) > curious - should the user be able to cause an abort just on command line misuse like that? My understanding is that assert (and therefore assert_no_error and error_abort) should be limited to fatal errors indicating qemu source bugs. Is it ok to report-and-abort a non recoverable error like this one? If so, theres significant cleanup we can do as many of us have been using the verbose error-report -> exit(1) for situations much like this. > After: > > Property '.foo' not foundAborted (core dumped) > > Note the loss of timestamp, name of executable, location, and the final > newline. Please fix. > > Amazing super-secret trick to get error messages fit for human > consumption: reproduce them before you commit! ;-P > Short series on list that straightens it all out based on your stub recommendations. Regards, Peter
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > On Wed, Jan 15, 2014 at 7:55 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >>> On Tue, 14 Jan 2014 17:44:51 +0100 >>> Kevin Wolf <kwolf@redhat.com> wrote: >>> >>>> Am 14.01.2014 um 04:38 hat Edgar E. Iglesias geschrieben: >>>> > On Tue, Jan 14, 2014 at 09:27:10AM +1000, Peter Crosthwaite wrote: >>>> > > Ping, >>>> > > >>>> > > Has this one been forgotten or are there issues? PMM had a small >>>> > > comment, but he waived it AFAICT. >>>> > >>>> > Pong, >>>> > >>>> > I've merged it now, thanks! >>>> >>>> I believe it's something in this pull requests that breaks make check. >>> >>> And you're right. But first, let me confirm that we're talking about the >>> same breakage. This is what I'm getting: >>> >>> make tests/check-qom-interface >>> libqemuutil.a(qemu-error.o): In function `error_vprintf': >>> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:23: undefined reference to `cur_mon' >>> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `cur_mon' >>> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:24: undefined reference to `monitor_vprintf' >>> libqemuutil.a(qemu-error.o): In function `error_printf_unless_qmp': >>> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:47: undefined reference to `monitor_cur_is_qmp' >>> libqemuutil.a(qemu-error.o): In function `error_print_loc': >>> /home/lcapitulino/work/src/upstream/qmp-unstable/util/qemu-error.c:174: undefined reference to `cur_mon' >>> collect2: error: ld returned 1 exit status >>> make: *** [tests/check-qom-interface] Error 1 >>> >>> I tried bisecting it, but git bisect weren't capable of finding the >>> culprit. So debugged it, and the problem was introduced by: >>> >>> commit 594278718323ca7bffaab0fb7fc6c82fa2c1cd5f >>> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> Date: Wed Jan 1 18:49:52 2014 -0800 >>> >>> qerror: Remove assert_no_error() >>> >> >> Are you sure you don't mean commit 5d24ee7 "error: Add error_abort"? >> >>> There isn't nothing really wrong with this commit. The problem seems to >>> be that the tests link against libqemuutil.a and this library pulls in >>> everything from util/. The commit above changed util/error.c to call >>> error_report(), >> >> Yes, 5d24ee7 does that. >> >>> which depends on 'cur_mon', which is only made available >>> by monitor.o. >> >> And stubs/mon-set-error.o >> >>> I don't think we want to mess up with including monitor.o on libqemuutil.a. >>> Besides, I now find it a bit weird to call error_report() from an error >>> reporting function. So it's better to just call fprintf(stderr,) instead. >> >> It's not weird at all. Higher layer calls lower layer. >> >>> Peter, Markus, are you ok with this patch? >>> >>> PS: I do run make check before sending a pull request, and did run this >>> time too. Not sure how I didn't catch this. Thanks for the report >>> Kevin! >>> >>> 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; >> >> Error message screwed up! >> >> Before: >> >> $ qemu -msg timestamp=on -global i440FX-pcihost.foo=bar >> 2014-01-15T09:50:18.066725Z upstream-qemu: Property '.foo' not found >> Aborted (core dumped) >> > > curious - should the user be able to cause an abort just on command > line misuse like that? My understanding is that assert (and therefore > assert_no_error and error_abort) should be limited to fatal errors > indicating qemu source bugs. Is it ok to report-and-abort a non > recoverable error like this one? If so, theres significant cleanup we > can do as many of us have been using the verbose error-report -> > exit(1) for situations much like this. Your understanding is correct: assertions are not an acceptable error reporting mechanism. The code is clearly abusing error_abort here. error_report() produces consistently formatted error messages. Less important for genuine "this cannot happen" reports; the part I need there a message that leads me to the point of failure in the code, and a core dump. Straight assert() or fprintf(); abort() can do that. But as long as error_abort is used cavalierly, as my test case demonstrates, we better stick to error_report(). >> After: >> >> Property '.foo' not foundAborted (core dumped) >> >> Note the loss of timestamp, name of executable, location, and the final >> newline. Please fix. >> >> Amazing super-secret trick to get error messages fit for human >> consumption: reproduce them before you commit! ;-P >> > > Short series on list that straightens it all out based on your stub > recommendations. Thanks!
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;