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

[Markus Armbruster]

Jes Sorensen <address@hidden> writes:

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

Looks good.

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

I guess that could make it actually convenient, because...

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

... you wouldn't have to pass around these max arguments then.

>           It is an attempt to be forward compatible for when it happens :)

Based on my own track record at predicting the future, I've come to
refrain from providing convenience features for future needs, in
particular when it complicates things.

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