From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byf84-00016N-NE for qemu-devel@nongnu.org; Mon, 24 Oct 2016 09:17:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byf7z-0005EP-KJ for qemu-devel@nongnu.org; Mon, 24 Oct 2016 09:17:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41892) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1byf7z-0005EH-CD for qemu-devel@nongnu.org; Mon, 24 Oct 2016 09:17:23 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B13794DD62 for ; Mon, 24 Oct 2016 13:17:22 +0000 (UTC) References: <1477301639-386-1-git-send-email-pbonzini@redhat.com> <20161024103400.GA4014@work-vm> <37942764-b3c8-1ac1-5121-894ada7300f2@redhat.com> <87vawhevxp.fsf@dusky.pond.sub.org> From: Paolo Bonzini Message-ID: <477e068b-d445-2296-25c6-976be1e2e319@redhat.com> Date: Mon, 24 Oct 2016 15:17:17 +0200 MIME-Version: 1.0 In-Reply-To: <87vawhevxp.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Dr. David Alan Gilbert" , qemu-devel@nongnu.org On 24/10/2016 15:08, Markus Armbruster wrote: > Paolo Bonzini writes: >=20 >> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote: >>> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>>> Leave the implementation of error_printf, error_printf_unless_qmp >>>> and error_vprintf to libqemustub.a and monitor.c, so that we can >>>> remove the monitor_printf and monitor_vprintf stubs. >>>> >>>> Signed-off-by: Paolo Bonzini >>>> --- >>>> This should help shutting up the vmstate unit tests. >>> >>> Why does this make it any easier than my patch? >> >>> You're still going to need to add something stub specific to turn >>> the output on and off. >> >> It makes it possible to override the functions independent of the rest >> of util/qemu-error.c. You can implement the functions in the test, sim= ply as >> >> had_stderr_output =3D true; >> >> and then assert that had_stderr_output is false or true depending on t= he >> test. >=20 > I buy that when I see a test using it :) Ok, so should I rewrite the test-vmstate patch to do this? >> (It's also a useful starting point to fix the cur_mon race). >=20 > Uh, the fix for the cur_mon race is making it thread-local, isn't it? Or just old-school mutex. There is monitor_lock, let's make it protect cur_mon. Paolo >> Consider this an RFC. error_printf probably should stay in qemu-error= .c >> since it can always call error_vprintf. >> >> Paolo >> >>> Dave >>> >>>> monitor.c | 38 ++++++++++++++++++++++++++++++++++++++ >>>> stubs/Makefile.objs | 2 +- >>>> stubs/error-printf.c | 26 ++++++++++++++++++++++++++ >>>> stubs/mon-printf.c | 11 ----------- >>>> util/qemu-error.c | 38 -------------------------------------- >>>> 5 files changed, 65 insertions(+), 50 deletions(-) >>>> create mode 100644 stubs/error-printf.c >>>> delete mode 100644 stubs/mon-printf.c >>>> >>>> diff --git a/monitor.c b/monitor.c >>>> index b73a999..dab910f 100644 >>>> --- a/monitor.c >>>> +++ b/monitor.c >>>> @@ -3957,6 +3957,44 @@ static void monitor_readline_flush(void *opaq= ue) >>>> monitor_flush(opaque); >>>> } >>>> =20 >>>> +/* >>>> + * Print to current monitor if we have one, else to stderr. >>>> + * TODO should return int, so callers can calculate width, but that >>>> + * requires surgery to monitor_vprintf(). Left for another day. >>>> + */ >>>> +void error_vprintf(const char *fmt, va_list ap) >>>> +{ >>>> + if (cur_mon && !monitor_cur_is_qmp()) { >>>> + monitor_vprintf(cur_mon, fmt, ap); >>>> + } else { >>>> + vfprintf(stderr, fmt, ap); >>>> + } >>>> +} >>>> + >>>> +/* >>>> + * Print to current monitor if we have one, else to stderr. >>>> + * TODO just like error_vprintf() >>>> + */ >>>> +void error_printf(const char *fmt, ...) >>>> +{ >>>> + va_list ap; >>>> + >>>> + va_start(ap, fmt); >>>> + error_vprintf(fmt, ap); >>>> + va_end(ap); >>>> +} >>>> + >>>> +void error_printf_unless_qmp(const char *fmt, ...) >>>> +{ >>>> + va_list ap; >>>> + >>>> + if (!monitor_cur_is_qmp()) { >>>> + va_start(ap, fmt); >>>> + error_vprintf(fmt, ap); >>>> + va_end(ap); >>>> + } >>>> +} >>>> + >>>> static void __attribute__((constructor)) monitor_lock_init(void) >>>> { >>>> qemu_mutex_init(&monitor_lock); >=20 > monitor.c has >3400 SLOC. I'd consider a separate error-printf.c. >=20 >>>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs >>>> index c5850e8..044bb47 100644 >>>> --- a/stubs/Makefile.objs >>>> +++ b/stubs/Makefile.objs >>>> @@ -9,6 +9,7 @@ stub-obj-y +=3D clock-warp.o >>>> stub-obj-y +=3D cpu-get-clock.o >>>> stub-obj-y +=3D cpu-get-icount.o >>>> stub-obj-y +=3D dump.o >>>> +stub-obj-y +=3D error-printf.o >>>> stub-obj-y +=3D fdset-add-fd.o >>>> stub-obj-y +=3D fdset-find-fd.o >>>> stub-obj-y +=3D fdset-get-fd.o >>>> @@ -22,7 +23,6 @@ stub-obj-y +=3D is-daemonized.o >>>> stub-obj-y +=3D machine-init-done.o >>>> stub-obj-y +=3D migr-blocker.o >>>> stub-obj-y +=3D mon-is-qmp.o >>>> -stub-obj-y +=3D mon-printf.o >>>> stub-obj-y +=3D monitor-init.o >>>> stub-obj-y +=3D notify-event.o >>>> stub-obj-y +=3D qtest.o >>>> diff --git a/stubs/error-printf.c b/stubs/error-printf.c >>>> new file mode 100644 >>>> index 0000000..19f7dd0 >>>> --- /dev/null >>>> +++ b/stubs/error-printf.c >>>> @@ -0,0 +1,26 @@ >>>> +#include "qemu/osdep.h" >>>> +#include "qemu-common.h" >>>> +#include "qemu/error-report.h" >>>> + >>>> +void error_vprintf(const char *fmt, va_list ap) >>>> +{ >>>> + vfprintf(stderr, fmt, ap); >>>> +} >>>> + >>>> +void error_printf(const char *fmt, ...) >>>> +{ >>>> + va_list ap; >>>> + >>>> + va_start(ap, fmt); >>>> + error_vprintf(fmt, ap); >>>> + va_end(ap); >>>> +} >>>> + >>>> +void error_printf_unless_qmp(const char *fmt, ...) >>>> +{ >>>> + va_list ap; >>>> + >>>> + va_start(ap, fmt); >>>> + error_vprintf(fmt, ap); >>>> + va_end(ap); >>>> +} >=20 > Copy of the non-stub code partially evaluated for !cur_mon && > !monitor_cur_is_qmp(). Okay if the copy earns its keep. It can't unti= l > it has actual users :) >=20 >>>> diff --git a/stubs/mon-printf.c b/stubs/mon-printf.c >>>> deleted file mode 100644 >>>> index e7c1e0c..0000000 >>>> --- a/stubs/mon-printf.c >>>> +++ /dev/null >>>> @@ -1,11 +0,0 @@ >>>> -#include "qemu/osdep.h" >>>> -#include "qemu-common.h" >>>> -#include "monitor/monitor.h" >>>> - >>>> -void monitor_printf(Monitor *mon, const char *fmt, ...) >>>> -{ >>>> -} >>>> - >>>> -void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) >>>> -{ >>>> -} >>>> diff --git a/util/qemu-error.c b/util/qemu-error.c >>>> index 1ef3566..dffd781 100644 >>>> --- a/util/qemu-error.c >>>> +++ b/util/qemu-error.c >>>> @@ -14,44 +14,6 @@ >>>> #include "monitor/monitor.h" >>>> #include "qemu/error-report.h" >>>> =20 >>>> -/* >>>> - * Print to current monitor if we have one, else to stderr. >>>> - * TODO should return int, so callers can calculate width, but that >>>> - * requires surgery to monitor_vprintf(). Left for another day. >>>> - */ >>>> -void error_vprintf(const char *fmt, va_list ap) >>>> -{ >>>> - if (cur_mon && !monitor_cur_is_qmp()) { >>>> - monitor_vprintf(cur_mon, fmt, ap); >>>> - } else { >>>> - vfprintf(stderr, fmt, ap); >>>> - } >>>> -} >>>> - >>>> -/* >>>> - * Print to current monitor if we have one, else to stderr. >>>> - * TODO just like error_vprintf() >>>> - */ >>>> -void error_printf(const char *fmt, ...) >>>> -{ >>>> - va_list ap; >>>> - >>>> - va_start(ap, fmt); >>>> - error_vprintf(fmt, ap); >>>> - va_end(ap); >>>> -} >>>> - >>>> -void error_printf_unless_qmp(const char *fmt, ...) >>>> -{ >>>> - va_list ap; >>>> - >>>> - if (!monitor_cur_is_qmp()) { >>>> - va_start(ap, fmt); >>>> - error_vprintf(fmt, ap); >>>> - va_end(ap); >>>> - } >>>> -} >>>> - >>>> static Location std_loc =3D { >>>> .kind =3D LOC_NONE >>>> }; >>>> --=20 >>>> 2.7.4 >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >>>