| Message ID | 1304674751-19111-2-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 6 May 2011 07:39:10 PM Jes.Sorensen@redhat.com 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? Brad
On 05/06/11 12:40, Brad Hards wrote: > On Fri, 6 May 2011 07:39:10 PM Jes.Sorensen@redhat.com 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. Cheers, Jes
Jes Sorensen <Jes.Sorensen@redhat.com> writes: > On 05/06/11 12:40, Brad Hards wrote: >> On Fri, 6 May 2011 07:39:10 PM Jes.Sorensen@redhat.com 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.
On 05/06/11 17:10, Markus Armbruster wrote: > Jes Sorensen <Jes.Sorensen@redhat.com> 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
Jes Sorensen <Jes.Sorensen@redhat.com> writes: > On 05/06/11 17:10, Markus Armbruster wrote: >> Jes Sorensen <Jes.Sorensen@redhat.com> 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
On 05/06/2011 06:10 PM, Markus Armbruster wrote: > 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. > */ My personal preference is to use fractions of 1 in the code and only covert to percentages during actual output.
diff --git a/qemu-progress.c b/qemu-progress.c index a4894c0..70928d6 100644 --- a/qemu-progress.c +++ b/qemu-progress.c @@ -111,6 +111,14 @@ void qemu_progress_end(void) state.end(); } +/* + * 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) { float current;