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

[Markus Armbruster]

Jes Sorensen <address@hidden> writes:

> On 05/06/11 12:40, Brad Hards wrote:
>> On Fri, 6 May 2011 07:39:10 PM address@hidden wrote:
>>> +/*
>>> + * Add delta to current state, and print the output if the current
>>> + * state has progressed more than min_skip since the last value was
>>> + * printed. 'max' specifies the relative percentage, ie. a function
>>> + * can count for 30% of the total work, and still count from 0-100, by
>>> + * setting max to 30. If max is set to zero, the percent argument
>>> + * becomes an absolute value for current state.
>>> + */
>>>  void qemu_progress_print(float percent, int max)
>> I hate to critique anyone adding docs, but this makes no sense at all to me 
>> without reading the code. Is "percent" the amount we are adding (i.e. the 
>> delta) or the result (i.e. absolute progress)? Or does it vary according to 
>> the value of max?
>
> 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.
 */

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.
 */

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)

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