[V2,4/5] util: add new function message_printf()

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(-)

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