[Qemu-devel] [PATCH qemu-block 0/2] cleanup progress code

[Qemu-devel] [PATCH qemu-block 0/2] cleanup progress code

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

Two small patches cleaning up the progress printing code - adding
documentation and removing some unneeded paranthesis. Also know as the
'happy markus' patch series....

This is relative to the block branch.

Jes

Jes Sorensen (2):
  Add documentation for qemu_progres_print()
  qemu-img.c: Remove superfluous parenthesis

 qemu-img.c      |    6 +++---
 qemu-progress.c |    8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.7.4.4

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

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-progress.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

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

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

[Brad Hards]

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

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

[Jes Sorensen]

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

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

[Markus Armbruster]

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.

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

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

[Markus Armbruster]

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

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 2/2] qemu-img.c: Remove superfluous parenthesis

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e825123..1da5484 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -785,7 +785,7 @@ static int img_convert(int argc, char **argv)
 
         nb_sectors = total_sectors;
         local_progress = (float)100 /
-            (nb_sectors / MIN(nb_sectors, (cluster_sectors)));
+            (nb_sectors / MIN(nb_sectors, cluster_sectors));
 
         for(;;) {
             int64_t bs_num;
@@ -856,7 +856,7 @@ static int img_convert(int argc, char **argv)
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
         local_progress = (float)100 /
-            (nb_sectors / MIN(nb_sectors, (IO_BUF_SIZE / 512)));
+            (nb_sectors / MIN(nb_sectors, IO_BUF_SIZE / 512));
 
         for(;;) {
             nb_sectors = total_sectors - sector_num;
@@ -1331,7 +1331,7 @@ static int img_rebase(int argc, char **argv)
         bdrv_get_geometry(bs, &num_sectors);
 
         local_progress = (float)100 /
-            (num_sectors / MIN(num_sectors, (IO_BUF_SIZE / 512)));
+            (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
         for (sector = 0; sector < num_sectors; sector += n) {
 
             /* How many sectors can we handle with the next read? */
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH qemu-block 0/2] cleanup progress code

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Hi,
>
> Two small patches cleaning up the progress printing code - adding
> documentation and removing some unneeded paranthesis. Also know as the
> 'happy markus' patch series....

Happy ACK :)