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

[Jes Sorensen <Jes.Sorensen@redhat.com>]

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