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

[Peter Crosthwaite]

On Wed, Jan 15, 2014 at 8:26 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Wed, Jan 15, 2014 at 4:22 AM, Luiz Capitulino <address@hidden> wrote:
>> 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()
>>
>> 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 <address@hidden>
>

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 <address@hidden>
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;
>>