Re: [Qemu-devel] [PATCH RFC 12/48] error: New error_printf() and error_vprintf()

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Mon, 01 Mar 2010 09:54:32 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Wed, 24 Feb 2010 18:55:24 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> FIXME They should return int, so callers can calculate width.
>> >> 
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  qemu-error.c |   49 ++++++++++++++++++++++++++++++++++++++++++-------
>> >>  qemu-error.h |   14 ++++++++++++++
>> >>  2 files changed, 56 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/qemu-error.c b/qemu-error.c
>> >> index 63bcdcf..76c660a 100644
>> >> --- a/qemu-error.c
>> >> +++ b/qemu-error.c
>> >> @@ -1,18 +1,53 @@
[...]
>> >> +/*
>> >> + * Print to current monitor if we have one, else to stderr.
>> >> + * FIXME should return int, so callers can calculate width, but that
>> >> + * requires surgery to monitor_printf().  Left for another day.
>> >> + */
>> >> +void error_vprintf(const char *fmt, va_list ap)
>> >>  {
>> >> -    va_list args;
>> >> -
>> >> -    va_start(args, fmt);
>> >>      if (cur_mon) {
>> >> -        monitor_vprintf(cur_mon, fmt, args);
>> >> +        monitor_vprintf(cur_mon, fmt, ap);
>> >>      } else {
>> >> -        vfprintf(stderr, fmt, args);
>> >> +        vfprintf(stderr, fmt, ap);
>> >>      }
>> >> -    va_end(args);
>> >> +}
>> >
>> >  This can be static.
>> 
>> Yes.  But why would that be useful?  It's neither a name space pollution
>> nor does it poke a hole into an abstraction.
>
>  Well, IMHO unused public symbols serve only one purpose: to pollute the
> global namespace :)
>
>  So, I think the question is: if it doesn't have any user and if you
> don't expect it to be used anytime soon: why make it public?

It's a basic building block.  Uses will come.

>> >> +
>> >> +/*
>> >> + * Print to current monitor if we have one, else to stderr.
>> >> + * FIXME just like error_vprintf()
>> >> + */
>> >> +void error_printf(const char *fmt, ...)
>> >> +{
>> >> +    va_list ap;
>> >> +
>> >> +    va_start(ap, fmt);
>> >> +    error_vprintf(fmt, ap);
>> >> +    va_end(ap);
>> >> +}
>> >
>> >  This function's name is inconsistent with qemu_error() and
>> > qemu_error_new().
>> 
>> I'm fond of prepending qemu_ to random symbols left and right.  Yes, I
>> know I'm reading QEMU source code, thank you :)
>> 
>> If the names here are really important: What about stripping qemu_ from
>> qemu_error() & friends?
>
>  I'm ok with that (and Paolo gave some suggestions), but I hope you
> submit a patch soon. It's ok to criticize/improve bad consistency policies,
> but it's not ok to break them.

Paolo's error_raise() works for me, although I like error_report() a bit
better.

But we need two names, one for simple, direct error reporting (now
qemu_error()), and one for QMP-compatible error reporting (now
qemu_error_new()).

Call them error_report() and qerror_report()?  Or is that too similar?