[PATCH 2/2] tools: virsh-domain: display progress with enhanced granularity

Shaleen Bathla posted 2 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH 2/2] tools: virsh-domain: display progress with enhanced granularity
Posted by Shaleen Bathla 2 years, 9 months ago
Switch from int to double for displaying job progress upto 2 decimal
places.

Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com>
---
 tools/virsh-domain.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 165aa0ee0f19..9f82722b2ac6 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1722,21 +1722,21 @@ static void
 virshPrintJobProgress(const char *label, unsigned long long remaining,
                       unsigned long long total)
 {
-    int progress = 100;
+    double progress = 100.0;
 
     /* if remaining == 0 migration has completed */
     if (remaining != 0) {
-        /* use float to avoid overflow */
-        progress = (int)(100.0 - remaining * 100.0 / total);
-        if (progress >= 100) {
+        /* use double to avoid overflow */
+        progress = 100.0 - (remaining * 100.0 / total);
+        if (progress >= 100.0) {
             /* migration has not completed, do not print [100 %] */
-            progress = 99;
+            progress = 99.0;
         }
     }
 
     /* see comments in vshError about why we must flush */
     fflush(stdout);
-    fprintf(stderr, "\r%s: [%3d %%]", label, progress);
+    fprintf(stderr, "\r%s: [%5.2f %%]", label, (int)(progress*100)/100.0);
     fflush(stderr);
 }
 
-- 
2.31.1
Re: [PATCH 2/2] tools: virsh-domain: display progress with enhanced granularity
Posted by Martin Kletzander 2 years, 9 months ago
On Wed, Apr 26, 2023 at 10:56:56AM +0530, Shaleen Bathla wrote:
>Switch from int to double for displaying job progress upto 2 decimal
>places.
>
>Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com>
>---
> tools/virsh-domain.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>index 165aa0ee0f19..9f82722b2ac6 100644
>--- a/tools/virsh-domain.c
>+++ b/tools/virsh-domain.c
>@@ -1722,21 +1722,21 @@ static void
> virshPrintJobProgress(const char *label, unsigned long long remaining,
>                       unsigned long long total)
> {
>-    int progress = 100;
>+    double progress = 100.0;
>
>     /* if remaining == 0 migration has completed */
>     if (remaining != 0) {
>-        /* use float to avoid overflow */
>-        progress = (int)(100.0 - remaining * 100.0 / total);
>-        if (progress >= 100) {
>+        /* use double to avoid overflow */
>+        progress = 100.0 - (remaining * 100.0 / total);

The parentheses are redundant here, but that's fine.

>+        if (progress >= 100.0) {
>             /* migration has not completed, do not print [100 %] */
>-            progress = 99;
>+            progress = 99.0;

Maybe we want 99.99 here?

>         }
>     }
>
>     /* see comments in vshError about why we must flush */
>     fflush(stdout);
>-    fprintf(stderr, "\r%s: [%3d %%]", label, progress);
>+    fprintf(stderr, "\r%s: [%5.2f %%]", label, (int)(progress*100)/100.0);

So this one took me a while, but I might've figured it out.  Are you
doing this so that the progress is not rounded up to 100.00 without
linking against math library?  If yes, then I'd suggest adding a comment
here just so we know once we look at it again in few years time.

Other than that the patches look fine to me.

>     fflush(stderr);
> }
>
>-- 
>2.31.1
>
Re: [PATCH 2/2] tools: virsh-domain: display progress with enhanced granularity
Posted by Michal Prívozník 2 years, 9 months ago
On 4/26/23 13:05, Martin Kletzander wrote:
> On Wed, Apr 26, 2023 at 10:56:56AM +0530, Shaleen Bathla wrote:
>> Switch from int to double for displaying job progress upto 2 decimal
>> places.
>>
>> Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com>
>> ---
>> tools/virsh-domain.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 165aa0ee0f19..9f82722b2ac6 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -1722,21 +1722,21 @@ static void
>> virshPrintJobProgress(const char *label, unsigned long long remaining,
>>                       unsigned long long total)
>> {
>> -    int progress = 100;
>> +    double progress = 100.0;
>>
>>     /* if remaining == 0 migration has completed */
>>     if (remaining != 0) {
>> -        /* use float to avoid overflow */
>> -        progress = (int)(100.0 - remaining * 100.0 / total);
>> -        if (progress >= 100) {
>> +        /* use double to avoid overflow */
>> +        progress = 100.0 - (remaining * 100.0 / total);
> 
> The parentheses are redundant here, but that's fine.
> 
>> +        if (progress >= 100.0) {
>>             /* migration has not completed, do not print [100 %] */
>> -            progress = 99;
>> +            progress = 99.0;
> 
> Maybe we want 99.99 here?
> 
>>         }
>>     }
>>
>>     /* see comments in vshError about why we must flush */
>>     fflush(stdout);
>> -    fprintf(stderr, "\r%s: [%3d %%]", label, progress);
>> +    fprintf(stderr, "\r%s: [%5.2f %%]", label,
>> (int)(progress*100)/100.0);
> 
> So this one took me a while, but I might've figured it out.  Are you
> doing this so that the progress is not rounded up to 100.00 without
> linking against math library?  If yes, then I'd suggest adding a comment
> here just so we know once we look at it again in few years time.

We already link with math library and for a good reason: we do use it
internally:

  libvirt.git $ git grep math\.h

but I don't mind this code.

Michal