Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Mon, 01 Mar 2010 09:54:32 +0100
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>
>> > On Wed, 24 Feb 2010 18:55:24 +0100
>> > Markus Armbruster <arm...@redhat.com> wrote:
>> >
>> >> FIXME They should return int, so callers can calculate width.
>> >>
>> >> Signed-off-by: Markus Armbruster <arm...@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 @@
[...]
>> >> +/*
>> >> + * 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?
