Re: [Qemu-devel] Fix make check breakage (was [PULL 00/14] QMP queue)

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> On Wed, Jan 15, 2014 at 7:55 PM, Markus Armbruster <address@hidden> wrote:
>> Luiz Capitulino <address@hidden> writes:
>>
>>> On Tue, 14 Jan 2014 17:44:51 +0100
>>> Kevin Wolf <address@hidden> 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 <address@hidden>
>>>   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!