Re: [Qemu-devel] [PATCH 1/2] Add documentation for qemu_progres_print()

[Jes Sorensen]

On 05/06/11 17:10, Markus Armbruster wrote:
> Jes Sorensen <address@hidden> writes:
>> What you add is a delta, which is relative to the max. We can change the
>> argument name of the function to be delta instead if that makes it
>> easier to follow.
> 
> Here's my try:
> 
> /*
>  * Report progress.
>  * @percent is how much progress we made.
>  * If @max is zero, @percent is how much of the job is done.
>  * Else, @percent is a progress delta since the last call, as a fraction
>  * of @max.  I.e. delta is @percent * @max / 100.  This is for
>  * convenience, it lets you account for @max% of the total work in some
>  * function, and still count @percent from 0 to 100.
>  */

Thanks! I made it a little more explicit based on your input:

/*
 * Report progress.
 * @delta is how much progress we made.
 * If @max is zero, @delta is an absolut value of the total job done.
 * Else, @delta is a progress delta since the last call, as a fraction
 * of @max.  I.e. the delta is @delta * @max / 100. This allows
 * relative accounting of functions which may be a different fraction of
 * the full job, depending on the context they are called in. I.e.
 * a function might be considered 40% of the full job if used from
 * bdrv_img_create() but only 20% if called from img_convert().
 */

> Document min_skip with qemu_progress_init():
> 
> /*
>  * Initialize progress reporting.
>  * If @enabled is false, actual reporting is suppressed.  The user can
>  * still trigger a report by sending SIGUSR1.
>  * Reports are also suppressed unless we've had at least @min_skip
>  * percent progress since the last report.
>  */

excellent!

> To be honest, the @max feature is a pain to document.  I'd ditch it.
> 
> For non-zero max,
> 
>     qemu_progress_report(x, max)
> 
> becomes
> 
>     qemu_progress_report(x * max/100)

I have to say I prefer having the max setting here - what would be an
option would be to keep the max value as a state, and then have a
qemu_progress_set_current_max() or something like that, so you wouldn't
have to type it every time?

> Not much of an inconvenience, in my opinion.  None at all when max is
> 100, which is the case for all non-zero max arguments so far.

The reason for introducing this is that some functions are called in
different ways, and to keep the same accounting code, it would be
possible to add the 'max' argument to those functions when they do their
counting. It is an attempt to be forward compatible for when it happens :)

> The only use of the absolute feature (zero max) so far is setting it to
> "all done", like this:
> 
>     qemu_progress_print(100, 0);
> 
> Can be done just as well with a delta >= 100, e.g.
> 
>     qemu_progress_print(100);
> 
> If a need for setting other absolute progress arises, I'd consider
> adding second function.

Getting rid of the absolute setting would be fine with me. You're right
that it is quite easy to set it that way.

Cheers,
Jes