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

[Luiz Capitulino]

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 @@
> >> +/*
> >> + * Error reporting
> >> + *
> >> + * Copyright (C) 2010 Red Hat Inc.
> >> + *
> >> + * Authors:
> >> + *  Markus Armbruster <address@hidden>,
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or 
> >> later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >>  #include <stdio.h>
> >>  #include "monitor.h"
> >>  #include "sysemu.h"
> >>  
> >> -void qemu_error(const char *fmt, ...)
> >> +/*
> >> + * 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?

> >> +
> >> +/*
> >> + * 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.