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

[Paolo Bonzini <pbonzini@redhat.com>]

On 03/01/2010 09:54 AM, Markus Armbruster wrote:
> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>
>> On Wed, 24 Feb 2010 18:55:24 +0100
>> Markus Armbruster<armbru@redhat.com>  wrote:
>>
>>> FIXME They should return int, so callers can calculate width.
>>>
>>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>>> ---
>>>   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<armbru@redhat.com>,
>>> + *
>>> + * 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