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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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


-- 
Best Regards

Wenchao Xia