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

[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 <address@hidden>
> ---
>  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.

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

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.

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

signature.asc
Description: OpenPGP digital signature