Re: [Qemu-devel] [PATCH v2 2/7] qemu-img: Add progress output for amend

[Max Reitz]

On 15.08.2014 13:18, Benoît Canet wrote:
> The Saturday 02 Aug 2014 à 01:49:16 (+0200), Max Reitz wrote :
> > Now that bdrv_amend_options() supports a status callback, use it to
> > display a progress report.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   qemu-img-cmds.hx |  4 ++--
> >   qemu-img.c       | 25 ++++++++++++++++++++++---
> >   qemu-img.texi    |  2 +-
> >   3 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 55aec6b..d83f3d0 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -70,8 +70,8 @@ STEXI
> >   ETEXI
> >   
> >   DEF("amend", img_amend,
> > -    "amend [-q] [-f fmt] [-t cache] -o options filename")
> > +    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
> >   STEXI
> > address@hidden amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} 
> > @var{filename}
> > address@hidden amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} 
> > @var{filename}
> >   @end table
> >   ETEXI
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 90d6b79..4b6406b 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -2732,6 +2732,12 @@ out:
> >       return 0;
> >   }
> >   
> > +static void amend_status_cb(BlockDriverState *bs,
> > +                            int64_t offset, int64_t total_work_size)
> > +{
> > +    qemu_progress_print(100.f * offset / total_work_size, 0);
> > +}
> > +
> >   static int img_amend(int argc, char **argv)
> >   {
> >       int c, ret = 0;
> > @@ -2740,12 +2746,12 @@ static int img_amend(int argc, char **argv)
> >       QemuOpts *opts = NULL;
> >       const char *fmt = NULL, *filename, *cache;
> >       int flags;
> > -    bool quiet = false;
> > +    bool quiet = false, progress = false;
> >       BlockDriverState *bs = NULL;
> >   
> >       cache = BDRV_DEFAULT_CACHE;
> >       for (;;) {
> > -        c = getopt(argc, argv, "ho:f:t:q");
> > +        c = getopt(argc, argv, "ho:f:t:pq");
> >           if (c == -1) {
> >               break;
> >           }
> > @@ -2775,6 +2781,9 @@ static int img_amend(int argc, char **argv)
> >               case 't':
> >                   cache = optarg;
> >                   break;
> > +            case 'p':
> > +                progress = true;
> > +                break;
> >               case 'q':
> >                   quiet = true;
> >                   break;
> > @@ -2785,6 +2794,11 @@ static int img_amend(int argc, char **argv)
> >           error_exit("Must specify options (-o)");
> >       }
> >   
> > +    if (quiet) {
> > +        progress = false;
> > +    }
> > +    qemu_progress_init(progress, 1.0);
> > +
> >       filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
> >       if (fmt && has_help_option(options)) {
> >           /* If a format is explicitly specified (and possibly no filename is
> > @@ -2827,13 +2841,18 @@ static int img_amend(int argc, char **argv)
> >           goto out;
> >       }
> >   
> > -    ret = bdrv_amend_options(bs, opts, NULL);
> > +    /* In case the driver does not call amend_status_cb() */
> > +    qemu_progress_print(0.f, 0);
> > +    ret = bdrv_amend_options(bs, opts, &amend_status_cb);
> > +    qemu_progress_print(100.f, 0);
> >       if (ret < 0) {
> >           error_report("Error while amending options: %s", strerror(-ret));
> >           goto out;
> >       }
> >   
> >   out:
> > +    qemu_progress_end();
> > +
> >       if (bs) {
> >           bdrv_unref(bs);
> >       }
> > diff --git a/qemu-img.texi b/qemu-img.texi
> > index cb68948..2c66603 100644
> > --- a/qemu-img.texi
> > +++ b/qemu-img.texi
> > @@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use 
> > file system and
> >   partitioning tools inside the VM to actually begin using the new space on the
> >   device.
> >   
> > address@hidden amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> > address@hidden amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} 
> > @var{filename}
> >   
> >   Amends the image format specific @var{options} for the image file
> >   @var{filename}. Not all file formats support this operation.
> > --
> > 2.0.3
> This patchs is fine and sweet.
> However could we add an assert(amend_status_cb) in bdrv_amend_options somewhere 
> in the series ?
> So if a coder pass NULL as callback he will have a clue near the root cause.

Judging from your response to patch 4, this seems no longer necessary...?

> Reviewed-by: Benoît Canet <address@hidden>

Just now seeing your new email address, hopefully I don't end up 
accidentally typing out the old one when adding the tag to commits. :-)

Max