| Message ID | 1267034160-3517-13-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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().
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?
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
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.
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?
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
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, ...)
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(-)