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

[Max Reitz <mreitz@redhat.com>]

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

> 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