Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()

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

于 2013-5-23 23:31, Eric Blake 写道:
> On 05/23/2013 02:47 AM, Wenchao Xia wrote:
>> Buffer is not used now so the string would not be truncated any more. They can be used
>> by both qemu and qemu-img with correct parameter specified.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   65 +++++++++++++++++++++++++++-----------------------
>>   include/block/qapi.h |    5 ++-
>>   qemu-img.c           |   15 +++++++----
>>   savevm.c             |   11 ++++++--
>>   4 files changed, 55 insertions(+), 41 deletions(-)
>>
>
>> @@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
>>                    (int)((secs / 60) % 60),
>>                    (int)(secs % 60),
>>                    (int)((sn->vm_clock_nsec / 1000000) % 1000));
>> -        snprintf(buf, buf_size,
>> -                 "%-10s%-20s%7s%20s%15s",
>> -                 sn->id_str, sn->name,
>> -                 get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
>> -                 date_buf,
>> -                 clock_buf);
>> +        message_printf(output,
>
> You got rid of ONE buffer...
>
>> +                       "%-10s%-20s%7s%20s%15s",
>> +                       sn->id_str, sn->name,
>> +                       get_human_readable_size(buf1, sizeof(buf1),
>
> ...but what is this other buffer still doing?  get_human_readable_size
> needs to be converted to use QemuOutput.
>
>> +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info)
>>   {
>>       char size_buf[128], dsize_buf[128];
>
> Why do we still need size_buf and dsize_buf?
>
>>       if (!info->has_actual_size) {
>> @@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info)
>>                                   info->actual_size);
>>       }
>>       get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
>
> Again, get_human_readable_size should be converted to use QemuOutput.
>
   They do not likely have a chance to be truncated, so have not change
them. I will convert them also in next version.

>> +++ b/qemu-img.c
>> @@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs)
>>   {
>>       QEMUSnapshotInfo *sn_tab, *sn;
>>       int nb_sns, i;
>> -    char buf[256];
>> +    QemuOutput output = {OUTPUT_STREAM, {stdout,} };
>
> This is relying on C99's rule that a union is initialized by its first
> named member.  But I think it might be more readable as:
>
> output = { .kind = OUTPUT_STREAM, .stream = stdout };
>
> not to mention that you will HAVE to use a designator to ever initialize
> the monitor element of the union in any parallel code that favors the
> monitor.
   This solve the initialization issue, will use it, thank u for the tip.

>
> Hmm, does C99 even allow anonymous unions, or is that a gcc extension?
>
> Overall, I like the direction this is headed.  The conversion looks
> reasonable, although it didn't quite go far enough for getting rid of
> buffers.
>


-- 
Best Regards

Wenchao Xia