Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

> 于 2013-4-11 20:05, Markus Armbruster 写道:
>> Wenchao Xia <address@hidden> writes:
>> 
>>>>>    
>>>>> -void bdrv_image_info_dump(ImageInfo *info)
>>>>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
>>>>>    {
>>>>>        char size_buf[128], dsize_buf[128];
>>>>>        if (!info->has_actual_size) {
>>>>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
>>>>
>>>> I don't like this change, because it introduces buffering for no
>>>> discernible reason.  Unless you can show me one, I'd like you to keep
>>>> printing directly.
>>>>
>>>    HMP code later need to call this function, and then print buf to
>>> monitor console, which is the goal of this patch.
>> 
>> Aha, the actual problem is "print either to stdout or to current
>> monitor", so that we can share code among qemu-img and monitor commands.
>> Such sharing is good, and the problem is real.
>> 
>> bdrv_snapshot_dump() solves it this way: print to a fixed-size buffer,
>> which the caller either prints to stdout (qemu-img) or to the current
>> monitor (info snapshots).
>> 
>> Your patch makes the buffer flexible.  Improvement.
>> 
>> But in my opinion, the detour through the buffer is silly.  Why not
>> simply use a print function that does the right thing?
>> 
>> We already have such a function for "print either to stderr or to
>> current monitor": error_printf().  Could easily add one for stdout.
>> 
>> [...]
>> 
>   Indeed it is a better way, how about:
> void message_printf(const char *fmt, ...);
> I am not sure where should this function be placed,
> hmp.c?

I figure it needs to go in a file that's linked into the utilities, too.
I think these are in make variables util-obj-y, stub-obj-y.

I'd be tempted to put it into qemu-error.c.  If you don't like that
because it's not about errors, consider renaming the file :)