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-24 9:48, Wenchao Xia 写道:
> 于 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.
>
   Just found this buffer is used in format control:

         message_printf(output,
                        "%-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);

   if get_human_readable_size() is converted, a manual control is
needed. Same thing happens to clock_buf. It seems better to keep what
it is.

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