Re: [Qemu-devel] [PATCH repost] qemu-img: Initial progress printing support

[Stefan Hajnoczi]

On Tue, Mar 29, 2011 at 9:51 AM,  <address@hidden> 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