Message ID | 1369298836-17416-5-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On 05/23/2013 02:47 AM, Wenchao Xia wrote: > This function takes an input parameter *output, which can be specified by > caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(), > which is a static function added in this patch. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > include/qemu/error-report.h | 13 +++++++++++++ > util/qemu-error.c | 28 ++++++++++++++++++++++++++-- > 2 files changed, 39 insertions(+), 2 deletions(-) > > +++ b/util/qemu-error.c > @@ -13,6 +13,25 @@ > #include <stdio.h> > #include "monitor/monitor.h" > > +static GCC_FMT_ATTR(2, 0) > +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap) > +{ > + if (output->kind == OUTPUT_STREAM) { > + vfprintf(output->stream, fmt, ap); > + } else if (output->kind == OUTPUT_MONITOR) { > + monitor_vprintf(output->monitor, fmt, ap); > + } Should you use a switch statement here, instead of open coding all possible enum values? But that's cosmetic. More importantly, I think this function should return an int, whose value is the value of vfprintf. On the monitor_vfprintf arm, it could return 0 for now (or, you could unravel THAT problem and fix monitor_vfprintf to return an output count, but that sounds like a bigger task). > +} > + > +void message_printf(const QemuOutput *output, const char *fmt, ...) > +{ > + va_list ap; > + > + va_start(ap, fmt); > + message_vprintf(output, fmt, ap); > + va_end(ap); This function should also return int. > +} > + > /* > * Print to current monitor if we have one, else to stderr. > * TODO should return int, so callers can calculate width, but that And by fixing the underlying function to return int, you could finally get rid of this TODO. Given that the int return problem is pre-existing, and probably deserves its own series, I'm fine with taking this patch as-is. Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, 23 May 2013 16:47:15 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > This function takes an input parameter *output, which can be specified by > caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(), > which is a static function added in this patch. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> This solution looks good to me: Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > include/qemu/error-report.h | 13 +++++++++++++ > util/qemu-error.c | 28 ++++++++++++++++++++++++++-- > 2 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > index c902cc1..cdde78b 100644 > --- a/include/qemu/error-report.h > +++ b/include/qemu/error-report.h > @@ -14,6 +14,8 @@ > #define QEMU_ERROR_H > > #include <stdarg.h> > +#include <stdio.h> > +#include "qemu/typedefs.h" > > typedef struct Location { > /* all members are private to qemu-error.c */ > @@ -32,6 +34,17 @@ void loc_set_none(void); > void loc_set_cmdline(char **argv, int idx, int cnt); > void loc_set_file(const char *fname, int lno); > > +typedef struct QemuOutput { > + enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind; > + union { > + FILE *stream; > + Monitor *monitor; > + }; > +} QemuOutput; > + > +void message_printf(const QemuOutput *output, const char *fmt, ...) > + GCC_FMT_ATTR(2, 3); > + > void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > diff --git a/util/qemu-error.c b/util/qemu-error.c > index 08a36f4..c7ff0a8 100644 > --- a/util/qemu-error.c > +++ b/util/qemu-error.c > @@ -13,6 +13,25 @@ > #include <stdio.h> > #include "monitor/monitor.h" > > +static GCC_FMT_ATTR(2, 0) > +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap) > +{ > + if (output->kind == OUTPUT_STREAM) { > + vfprintf(output->stream, fmt, ap); > + } else if (output->kind == OUTPUT_MONITOR) { > + monitor_vprintf(output->monitor, fmt, ap); > + } > +} > + > +void message_printf(const QemuOutput *output, const char *fmt, ...) > +{ > + va_list ap; > + > + va_start(ap, fmt); > + message_vprintf(output, fmt, ap); > + va_end(ap); > +} > + > /* > * Print to current monitor if we have one, else to stderr. > * TODO should return int, so callers can calculate width, but that > @@ -20,11 +39,16 @@ > */ > void error_vprintf(const char *fmt, va_list ap) > { > + QemuOutput output; > + > if (cur_mon) { > - monitor_vprintf(cur_mon, fmt, ap); > + output.kind = OUTPUT_MONITOR; > + output.monitor = cur_mon; > } else { > - vfprintf(stderr, fmt, ap); > + output.kind = OUTPUT_STREAM; > + output.stream = stderr; > } > + message_vprintf(&output, fmt, ap); > } > > /*
δΊ 2013-5-23 23:05, Eric Blake ει: > On 05/23/2013 02:47 AM, Wenchao Xia wrote: >> This function takes an input parameter *output, which can be specified by >> caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(), >> which is a static function added in this patch. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> include/qemu/error-report.h | 13 +++++++++++++ >> util/qemu-error.c | 28 ++++++++++++++++++++++++++-- >> 2 files changed, 39 insertions(+), 2 deletions(-) >> >> +++ b/util/qemu-error.c >> @@ -13,6 +13,25 @@ >> #include <stdio.h> >> #include "monitor/monitor.h" >> >> +static GCC_FMT_ATTR(2, 0) >> +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap) >> +{ >> + if (output->kind == OUTPUT_STREAM) { >> + vfprintf(output->stream, fmt, ap); >> + } else if (output->kind == OUTPUT_MONITOR) { >> + monitor_vprintf(output->monitor, fmt, ap); >> + } > > Should you use a switch statement here, instead of open coding all > possible enum values? But that's cosmetic. > > More importantly, I think this function should return an int, whose > value is the value of vfprintf. On the monitor_vfprintf arm, it could > return 0 for now (or, you could unravel THAT problem and fix > monitor_vfprintf to return an output count, but that sounds like a > bigger task). > >> +} >> + >> +void message_printf(const QemuOutput *output, const char *fmt, ...) >> +{ >> + va_list ap; >> + >> + va_start(ap, fmt); >> + message_vprintf(output, fmt, ap); >> + va_end(ap); > > This function should also return int. > >> +} >> + >> /* >> * Print to current monitor if we have one, else to stderr. >> * TODO should return int, so callers can calculate width, but that > > And by fixing the underlying function to return int, you could finally > get rid of this TODO. > > Given that the int return problem is pre-existing, and probably deserves > its own series, I'm fine with taking this patch as-is. > It may not have much meaning of returning int now, since underlining function do not support it so no caller can benefit from it. I think a series later for that is better, thanks for your reviewing. > Reviewed-by: Eric Blake <eblake@redhat.com> >
On Thu, May 23, 2013 at 04:47:15PM +0800, Wenchao Xia wrote: > This function takes an input parameter *output, which can be specified by > caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(), > which is a static function added in this patch. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > include/qemu/error-report.h | 13 +++++++++++++ > util/qemu-error.c | 28 ++++++++++++++++++++++++++-- > 2 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > index c902cc1..cdde78b 100644 > --- a/include/qemu/error-report.h > +++ b/include/qemu/error-report.h > @@ -14,6 +14,8 @@ > #define QEMU_ERROR_H > > #include <stdarg.h> > +#include <stdio.h> > +#include "qemu/typedefs.h" > > typedef struct Location { > /* all members are private to qemu-error.c */ > @@ -32,6 +34,17 @@ void loc_set_none(void); > void loc_set_cmdline(char **argv, int idx, int cnt); > void loc_set_file(const char *fname, int lno); > > +typedef struct QemuOutput { > + enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind; > + union { > + FILE *stream; > + Monitor *monitor; > + }; > +} QemuOutput; > + > +void message_printf(const QemuOutput *output, const char *fmt, ...) > + GCC_FMT_ATTR(2, 3); This is introducing a slightly different solution for fprintf_function, which is already widely used: $ git grep fprintf_function | wc -l 101 Please reuse fprintf_function. Stefan
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index c902cc1..cdde78b 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -14,6 +14,8 @@ #define QEMU_ERROR_H #include <stdarg.h> +#include <stdio.h> +#include "qemu/typedefs.h" typedef struct Location { /* all members are private to qemu-error.c */ @@ -32,6 +34,17 @@ void loc_set_none(void); void loc_set_cmdline(char **argv, int idx, int cnt); void loc_set_file(const char *fname, int lno); +typedef struct QemuOutput { + enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind; + union { + FILE *stream; + Monitor *monitor; + }; +} QemuOutput; + +void message_printf(const QemuOutput *output, const char *fmt, ...) + GCC_FMT_ATTR(2, 3); + void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2); void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); diff --git a/util/qemu-error.c b/util/qemu-error.c index 08a36f4..c7ff0a8 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -13,6 +13,25 @@ #include <stdio.h> #include "monitor/monitor.h" +static GCC_FMT_ATTR(2, 0) +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap) +{ + if (output->kind == OUTPUT_STREAM) { + vfprintf(output->stream, fmt, ap); + } else if (output->kind == OUTPUT_MONITOR) { + monitor_vprintf(output->monitor, fmt, ap); + } +} + +void message_printf(const QemuOutput *output, const char *fmt, ...) +{ + va_list ap; + + va_start(ap, fmt); + message_vprintf(output, fmt, ap); + va_end(ap); +} + /* * Print to current monitor if we have one, else to stderr. * TODO should return int, so callers can calculate width, but that @@ -20,11 +39,16 @@ */ void error_vprintf(const char *fmt, va_list ap) { + QemuOutput output; + if (cur_mon) { - monitor_vprintf(cur_mon, fmt, ap); + output.kind = OUTPUT_MONITOR; + output.monitor = cur_mon; } else { - vfprintf(stderr, fmt, ap); + output.kind = OUTPUT_STREAM; + output.stream = stderr; } + message_vprintf(&output, fmt, ap); } /*
This function takes an input parameter *output, which can be specified by caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(), which is a static function added in this patch. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- include/qemu/error-report.h | 13 +++++++++++++ util/qemu-error.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-)