Patchwork [RFC,12/48] error: New error_printf() and error_vprintf()

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 24, 2010, 5:55 p.m.
Message ID <1267034160-3517-13-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/46185/
State New
Headers show

Comments

Markus Armbruster - Feb. 24, 2010, 5:55 p.m.
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(-)
Luiz Capitulino - Feb. 26, 2010, 7:43 p.m.
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.

> +
> +/*
> + * 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().
Markus Armbruster - March 1, 2010, 8:54 a.m.
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?
Paolo Bonzini - March 1, 2010, 12:45 p.m.
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
Luiz Capitulino - March 1, 2010, 4:09 p.m.
On Mon, 01 Mar 2010 09:54:32 +0100
Markus Armbruster <armbru@redhat.com> 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.

 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.
Markus Armbruster - March 2, 2010, 8:33 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 01 Mar 2010 09:54:32 +0100
> Markus Armbruster <armbru@redhat.com> 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 @@
[...]
>> >> +/*
>> >> + * 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?
Paolo Bonzini - March 2, 2010, 12:37 p.m.
On 03/02/2010 09:33 AM, Markus Armbruster wrote:
> Paolo's error_raise() works for me, although I like error_report() a bit
> better.

error_report is also fine, of course.

> 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?

Actually I like it.

Paolo

Patch

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);
+}
+
+/*
+ * 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);
+}
+
+void qemu_error(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
 }
 
 void qemu_error_internal(const char *file, int linenr, const char *func,
diff --git a/qemu-error.h b/qemu-error.h
index fa16113..d90f1da 100644
--- a/qemu-error.h
+++ b/qemu-error.h
@@ -1,6 +1,20 @@ 
+/*
+ * 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.
+ */
+
 #ifndef QEMU_ERROR_H
 #define QEMU_ERROR_H
 
+void error_vprintf(const char *fmt, va_list ap);
+void error_printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
 void qemu_error(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
 void qemu_error_internal(const char *file, int linenr, const char *func,
                          const char *fmt, ...)