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

["Benoît Canet" <benoit.canet@irqsave.net>]

The Friday 15 Aug 2014 à 15:06:12 (+0200), Max Reitz wrote :
> 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 <mreitz@redhat.com>
> >>---
> >>  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
> >>-@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>+@item 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.
> >>-@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>+@item 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...?

I was thinking about avoiding adding burden to the qemu know how
(like "oh I added a field to BlockDriverState so I must modify bdrv_swap")
but I trust your jugement on this one.

> 
> >Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
> 
> 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
>