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

[Paolo Bonzini]

On 03/01/2010 09:54 AM, Markus Armbruster 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.
>
> > > +
> > > +/*
> > > + * 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?

Since we are at it, qemu_error_new could be renamed to qerror_raise or 
error_raise (since it returns void).  Not worthwhile if you're not 
changing the name already, but in that case...

Paolo