diff mbox

[repost] qemu-img: Initial progress printing support

Message ID 1301388675-2509-1-git-send-email-Jes.Sorensen@redhat.com
State New
Headers show

Commit Message

Jes Sorensen March 29, 2011, 8:51 a.m. UTC
From: Jes Sorensen <Jes.Sorensen@redhat.com>

This adds the basic infrastructure for supporting progress output
on the command line, as well as progress support for qemu-img commands
'rebase' and 'convert'.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Makefile.objs    |    2 +-
 qemu-common.h    |    5 +++
 qemu-img-cmds.hx |    4 +-
 qemu-img.c       |   38 ++++++++++++++++++++-
 qemu-progress.c  |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 138 insertions(+), 5 deletions(-)
 create mode 100644 qemu-progress.c

Comments

Stefan Hajnoczi March 30, 2011, 9:50 a.m. UTC | #1
On Tue, Mar 29, 2011 at 9:51 AM,  <Jes.Sorensen@redhat.com> wrote:
> diff --git a/qemu-common.h b/qemu-common.h
> index 7a96dd1..a3a4dde 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -330,6 +330,11 @@ void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count);
>  void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
>                             size_t skip);
>
> +void qemu_init_progress(int enabled, float min_skip);

Please call it qemu_progress_init() to be consistent with the other
qemu_progress_*() functions.

> @@ -642,6 +648,9 @@ static int img_convert(int argc, char **argv)
>         goto out;
>     }
>
> +    qemu_init_progress(progress, 2.0);
> +    qemu_progress_print(0, 100);
> +
>     bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
>
>     total_sectors = 0;
> @@ -773,6 +782,11 @@ static int img_convert(int argc, char **argv)
>         }
>         cluster_sectors = cluster_size >> 9;
>         sector_num = 0;
> +
> +        nb_sectors = total_sectors - sector_num;

sector_num is always 0 here, why subtract it?

> +        local_progress = (float)100 /
> +            (nb_sectors / MIN(nb_sectors, (cluster_sectors)));

This seems to calculate the percentage increment for each iteration.
This increment value is wrong for the final iteration unless
nb_sectors is a multiple of cluster_sectors, so we'll overshoot 100%.

It would be nice if the caller didn't have to calculate progress
themselves.  How about:

void qemu_progress_begin(bool enabled, uint64_t max);
void qemu_progress_end(void);
void qemu_progress_add(uint64_t increment);

You set the maximum absolute value and then tell it how much progress
has been made each iteration:
qemu_progress_begin(true, total_sectors);
for (...) {
    nbytes = ...;
    qemu_progress_add(nbytes);
}
qemu_progress_end();

> +void qemu_init_progress(int enabled, float min_skip)
> +{
> +    state.enabled = enabled;
> +    if (!enabled) {
> +        return;
> +    }

This early return is not needed.

> +    state.min_skip = min_skip;
> +}
> +
> +void qemu_progress_end(void)
> +{
> +    progress_simple_end();
> +}
> +
> +void qemu_progress_print(float percent, int max)
> +{
> +    float current;
> +
> +    if (max == 0) {
> +        current = percent;
> +    } else {
> +        current = state.current + percent / 100 * max;
> +    }
> +    state.current = current;
> +
> +    if (current > (state.last_print + state.min_skip) ||
> +        (current == 100) || (current == 0)) {

Comparing against (float)100 may not match due to floating point representation.

> +        state.last_print = state.current;
> +        progress_simple_print();
> +    }
> +}
> +
> +int qemu_progress_get_current(void)
> +{
> +    return state.current;
> +}

This function is unused.

Stefan
Jes Sorensen March 30, 2011, 12:07 p.m. UTC | #2
On 03/30/11 11:50, Stefan Hajnoczi wrote:
> On Tue, Mar 29, 2011 at 9:51 AM,  <Jes.Sorensen@redhat.com> wrote:
>> diff --git a/qemu-common.h b/qemu-common.h
>> index 7a96dd1..a3a4dde 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -330,6 +330,11 @@ void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count);
>>  void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
>>                             size_t skip);
>>
>> +void qemu_init_progress(int enabled, float min_skip);
> 
> Please call it qemu_progress_init() to be consistent with the other
> qemu_progress_*() functions.

Fixed

>> @@ -773,6 +782,11 @@ static int img_convert(int argc, char **argv)
>>         }
>>         cluster_sectors = cluster_size >> 9;
>>         sector_num = 0;
>> +
>> +        nb_sectors = total_sectors - sector_num;
> 
> sector_num is always 0 here, why subtract it?

Copy paste from another place where I calculated it - fixed.

>> +        local_progress = (float)100 /
>> +            (nb_sectors / MIN(nb_sectors, (cluster_sectors)));
> 
> This seems to calculate the percentage increment for each iteration.
> This increment value is wrong for the final iteration unless
> nb_sectors is a multiple of cluster_sectors, so we'll overshoot 100%.
>
> It would be nice if the caller didn't have to calculate progress
> themselves.  How about:
>
> void qemu_progress_begin(bool enabled, uint64_t max);
> void qemu_progress_end(void);
> void qemu_progress_add(uint64_t increment);
>
> You set the maximum absolute value and then tell it how much progress
> has been made each iteration:
> qemu_progress_begin(true, total_sectors);
> for (...) {
>     nbytes = ...;
>     qemu_progress_add(nbytes);
> }
> qemu_progress_end();

The overshoot is handled by checking for >= 100 and not going above it.
I designed the API to try and handle the case where you can have a
function being a portion of the bigger picture. So basically you could
have this:

   part1() 20%
   part2() 50%
   part3() 30%

But in another situation you might only run and expect the following split:

   part2() 60%
   part3() 40%

In that case you need to be able to calculate the relative number.
Obviously it requires part2() and part3() are given an argument
specifying how big of the big scheme they are. If we do it the way you
suggest with a fixed increment, this option is not really doable.

>> +void qemu_init_progress(int enabled, float min_skip)
>> +{
>> +    state.enabled = enabled;
>> +    if (!enabled) {
>> +        return;
>> +    }
> 
> This early return is not needed.

It was added if the function was going to get more complicated, but I
can pull it out.

>> +void qemu_progress_print(float percent, int max)
>> +{
>> +    float current;
>> +
>> +    if (max == 0) {
>> +        current = percent;
>> +    } else {
>> +        current = state.current + percent / 100 * max;
>> +    }
>> +    state.current = current;
>> +
>> +    if (current > (state.last_print + state.min_skip) ||
>> +        (current == 100) || (current == 0)) {
> 
> Comparing against (float)100 may not match due to floating point representation.

I'll add a check for >= 100 setting current to 100, that should fix it.

>> +int qemu_progress_get_current(void)
>> +{
>> +    return state.current;
>> +}
> 
> This function is unused.

It was added to make the API more complete, but lets just pull it out.

Thanks for the input! I'll post a v2 shortly

Jes
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index f8cf199..6bccea7 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,7 +14,7 @@  oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
 # block-obj-y is code used by both qemu system emulation and qemu-img
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o async.o
-block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
+block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o qemu-progress.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
diff --git a/qemu-common.h b/qemu-common.h
index 7a96dd1..a3a4dde 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -330,6 +330,11 @@  void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count);
 void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
                             size_t skip);
 
+void qemu_init_progress(int enabled, float min_skip);
+void qemu_progress_end(void);
+void qemu_progress_print(float percent, int max);
+int qemu_progress_get_current(void);
+
 /* Convert a byte between binary and BCD.  */
 static inline uint8_t to_bcd(uint8_t val)
 {
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 6c7176f..3072d38 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -28,7 +28,7 @@  STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-f fmt] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename")
+    "convert [-c] [-p] [-f fmt] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename")
 STEXI
 @item convert [-c] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
@@ -46,7 +46,7 @@  STEXI
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [-f fmt] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [-f fmt] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
 @item rebase [-f @var{fmt}] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 7e3cc4c..eadeebb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -77,6 +77,7 @@  static void help(void)
            "       match exactly. The image doesn't need a working backing file before\n"
            "       rebasing in this case (useful for renaming the backing file)\n"
            "  '-h' with or without a command shows this help and lists the supported formats\n"
+           "  '-p' show progress of command (only certain commands)\n"
            "\n"
            "Parameters to snapshot subcommand:\n"
            "  'snapshot' is the name of the snapshot to create, apply or delete\n"
@@ -567,6 +568,7 @@  static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
 static int img_convert(int argc, char **argv)
 {
     int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
+    int progress = 0;
     const char *fmt, *out_fmt, *out_baseimg, *out_filename;
     BlockDriver *drv, *proto_drv;
     BlockDriverState **bs = NULL, *out_bs = NULL;
@@ -579,13 +581,14 @@  static int img_convert(int argc, char **argv)
     QEMUOptionParameter *out_baseimg_param;
     char *options = NULL;
     const char *snapshot_name = NULL;
+    float local_progress;
 
     fmt = NULL;
     out_fmt = "raw";
     out_baseimg = NULL;
     compress = 0;
     for(;;) {
-        c = getopt(argc, argv, "f:O:B:s:hce6o:");
+        c = getopt(argc, argv, "f:O:B:s:hce6o:p");
         if (c == -1) {
             break;
         }
@@ -620,6 +623,9 @@  static int img_convert(int argc, char **argv)
         case 's':
             snapshot_name = optarg;
             break;
+        case 'p':
+            progress = 1;
+            break;
         }
     }
 
@@ -642,6 +648,9 @@  static int img_convert(int argc, char **argv)
         goto out;
     }
         
+    qemu_init_progress(progress, 2.0);
+    qemu_progress_print(0, 100);
+
     bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
 
     total_sectors = 0;
@@ -773,6 +782,11 @@  static int img_convert(int argc, char **argv)
         }
         cluster_sectors = cluster_size >> 9;
         sector_num = 0;
+
+        nb_sectors = total_sectors - sector_num;
+        local_progress = (float)100 /
+            (nb_sectors / MIN(nb_sectors, (cluster_sectors)));
+
         for(;;) {
             int64_t bs_num;
             int remainder;
@@ -832,6 +846,7 @@  static int img_convert(int argc, char **argv)
                 }
             }
             sector_num += n;
+            qemu_progress_print(local_progress, 100);
         }
         /* signal EOF to align */
         bdrv_write_compressed(out_bs, 0, NULL, 0);
@@ -839,6 +854,10 @@  static int img_convert(int argc, char **argv)
         int has_zero_init = bdrv_has_zero_init(out_bs);
 
         sector_num = 0; // total number of sectors converted so far
+        nb_sectors = total_sectors - sector_num;
+        local_progress = (float)100 /
+            (nb_sectors / MIN(nb_sectors, (IO_BUF_SIZE / 512)));
+
         for(;;) {
             nb_sectors = total_sectors - sector_num;
             if (nb_sectors <= 0) {
@@ -912,9 +931,11 @@  static int img_convert(int argc, char **argv)
                 n -= n1;
                 buf1 += n1 * 512;
             }
+            qemu_progress_print(local_progress, 100);
         }
     }
 out:
+    qemu_progress_end();
     free_option_parameters(create_options);
     free_option_parameters(param);
     qemu_free(buf);
@@ -1184,6 +1205,7 @@  static int img_rebase(int argc, char **argv)
     const char *fmt, *out_basefmt, *out_baseimg;
     int c, flags, ret;
     int unsafe = 0;
+    int progress = 0;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -1191,7 +1213,7 @@  static int img_rebase(int argc, char **argv)
     out_basefmt = NULL;
 
     for(;;) {
-        c = getopt(argc, argv, "uhf:F:b:");
+        c = getopt(argc, argv, "uhf:F:b:p");
         if (c == -1) {
             break;
         }
@@ -1212,6 +1234,9 @@  static int img_rebase(int argc, char **argv)
         case 'u':
             unsafe = 1;
             break;
+        case 'p':
+            progress = 1;
+            break;
         }
     }
 
@@ -1220,6 +1245,9 @@  static int img_rebase(int argc, char **argv)
     }
     filename = argv[optind++];
 
+    qemu_init_progress(progress, 2.0);
+    qemu_progress_print(0, 100);
+
     /*
      * Open the images.
      *
@@ -1295,12 +1323,15 @@  static int img_rebase(int argc, char **argv)
         int n;
         uint8_t * buf_old;
         uint8_t * buf_new;
+        float local_progress;
 
         buf_old = qemu_malloc(IO_BUF_SIZE);
         buf_new = qemu_malloc(IO_BUF_SIZE);
 
         bdrv_get_geometry(bs, &num_sectors);
 
+        local_progress = (float)100 /
+            (num_sectors / MIN(num_sectors, (IO_BUF_SIZE / 512)));
         for (sector = 0; sector < num_sectors; sector += n) {
 
             /* How many sectors can we handle with the next read? */
@@ -1348,6 +1379,7 @@  static int img_rebase(int argc, char **argv)
 
                 written += pnum;
             }
+            qemu_progress_print(local_progress, 100);
         }
 
         qemu_free(buf_old);
@@ -1368,6 +1400,7 @@  static int img_rebase(int argc, char **argv)
             out_baseimg, strerror(-ret));
     }
 
+    qemu_progress_print(100, 0);
     /*
      * TODO At this point it is possible to check if any clusters that are
      * allocated in the COW file are the same in the backing file. If so, they
@@ -1375,6 +1408,7 @@  static int img_rebase(int argc, char **argv)
      * backing file, in case of a crash this would lead to corruption.
      */
 out:
+    qemu_progress_end();
     /* Cleanup */
     if (!unsafe) {
         bdrv_delete(bs_old_backing);
diff --git a/qemu-progress.c b/qemu-progress.c
new file mode 100644
index 0000000..1fbbf6a
--- /dev/null
+++ b/qemu-progress.c
@@ -0,0 +1,94 @@ 
+/*
+ * QEMU progress printing utility functions
+ *
+ * Copyright (C) 2011 Jes Sorensen <Jes.Sorensen@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu-common.h"
+#include "osdep.h"
+#include "sysemu.h"
+#include <stdio.h>
+
+struct progress_state {
+    int enabled;
+    float current;
+    float last_print;
+    float min_skip;
+};
+
+static struct progress_state state;
+
+/*
+ * Simple progress print function.
+ * @percent relative percent of current operation
+ * @max percent of total operation
+ */
+static void progress_simple_print(void)
+{
+    if (state.enabled) {
+        printf("    (%3.2f/100%%)\r", state.current);
+        fflush(stdout);
+    }
+}
+
+static void progress_simple_end(void)
+{
+    if (state.enabled) {
+        printf("\n");
+    }
+}
+
+void qemu_init_progress(int enabled, float min_skip)
+{
+    state.enabled = enabled;
+    if (!enabled) {
+        return;
+    }       
+    state.min_skip = min_skip;
+}
+
+void qemu_progress_end(void)
+{
+    progress_simple_end();
+}
+
+void qemu_progress_print(float percent, int max)
+{
+    float current;
+
+    if (max == 0) {
+        current = percent;
+    } else {
+        current = state.current + percent / 100 * max;
+    }
+    state.current = current;
+
+    if (current > (state.last_print + state.min_skip) ||
+        (current == 100) || (current == 0)) {
+        state.last_print = state.current;
+        progress_simple_print();
+    }
+}
+
+int qemu_progress_get_current(void)
+{
+    return state.current;
+}