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

[Benoît Canet]

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.

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