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

[Wenchao Xia]

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