Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]