Patchwork [v2,3/5] qemu-img: avoid excessive BlockFragInfo line length

login
register
mail settings
Submitter Stefan Hajnoczi
Date Feb. 6, 2013, 10:58 a.m.
Message ID <1360148318-26988-4-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/218550/
State New
Headers show

Comments

Stefan Hajnoczi - Feb. 6, 2013, 10:58 a.m.
The qemu-img check printf() statement that shows BlockFragInfo results
is poorly formatted.  Introduce a local variable to shorten the lines
and restore proper indentation.

The next patch adds a field to BlockFragInfo so it is beneficial to
straighten out this code before modifying it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
Kevin Wolf - Feb. 6, 2013, 11:30 a.m.
Am 06.02.2013 11:58, schrieb Stefan Hajnoczi:
> The qemu-img check printf() statement that shows BlockFragInfo results
> is poorly formatted.  Introduce a local variable to shorten the lines
> and restore proper indentation.
> 
> The next patch adds a field to BlockFragInfo so it is beneficial to
> straighten out this code before modifying it.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 85d3740..167d65f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -389,6 +389,7 @@ static int img_check(int argc, char **argv)
>      const char *filename, *fmt;
>      BlockDriverState *bs;
>      BdrvCheckResult result;
> +    BlockFragInfo *bfi = &result.bfi;
>      int fix = 0;
>      int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
>  
> @@ -468,11 +469,12 @@ static int img_check(int argc, char **argv)
>          }
>      }
>  
> -    if (result.bfi.total_clusters != 0 && result.bfi.allocated_clusters != 0) {
> -        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n",
> -        result.bfi.allocated_clusters, result.bfi.total_clusters,
> -        result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters,
> -        result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters);
> +    if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
> +        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "

Can you add a space before the '=' while touching this?

Kevin
Eric Blake - Feb. 6, 2013, 1:35 p.m.
On 02/06/2013 04:30 AM, Kevin Wolf wrote:

>>  
>> -    if (result.bfi.total_clusters != 0 && result.bfi.allocated_clusters != 0) {
>> -        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n",
>> -        result.bfi.allocated_clusters, result.bfi.total_clusters,
>> -        result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters,
>> -        result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters);
>> +    if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
>> +        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
> 
> Can you add a space before the '=' while touching this?

Perhaps he can, but as this is user-visible output, and we don't yet
have a way to force JSON output to get at the same information,
arbitrarily changing the output format may break some existing user that
is scraping the output.  It would be nice if we had access to this
output information in a more stable format.
Kevin Wolf - Feb. 6, 2013, 1:45 p.m.
Am 06.02.2013 14:35, schrieb Eric Blake:
> On 02/06/2013 04:30 AM, Kevin Wolf wrote:
> 
>>>  
>>> -    if (result.bfi.total_clusters != 0 && result.bfi.allocated_clusters != 0) {
>>> -        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n",
>>> -        result.bfi.allocated_clusters, result.bfi.total_clusters,
>>> -        result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters,
>>> -        result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters);
>>> +    if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
>>> +        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
>>
>> Can you add a space before the '=' while touching this?
> 
> Perhaps he can, but as this is user-visible output, and we don't yet
> have a way to force JSON output to get at the same information,
> arbitrarily changing the output format may break some existing user that
> is scraping the output.  It would be nice if we had access to this
> output information in a more stable format.

Are you aware of any existing user that makes use of this information?

We're going to add JSON output soon (possibly even before this series
goes in) and scraping the output was never considered right for
'qemu-img check'. Also, so far this information only exists for QED
images, which makes it rather unlikely to be relied on. I think
modifying it is okay (if it wasn't, we would even have to think twice
about adding new fields).

Kevin
Eric Blake - Feb. 6, 2013, 2:56 p.m.
On 02/06/2013 06:45 AM, Kevin Wolf wrote:
>>> Can you add a space before the '=' while touching this?
>>
>> Perhaps he can, but as this is user-visible output, and we don't yet
>> have a way to force JSON output to get at the same information,
>> arbitrarily changing the output format may break some existing user that
>> is scraping the output.  It would be nice if we had access to this
>> output information in a more stable format.
> 
> Are you aware of any existing user that makes use of this information?

Not particularly, which is why I'm okay if we modify the output.  I was
just making sure we were thinking about the issue.

> 
> We're going to add JSON output soon (possibly even before this series
> goes in) and scraping the output was never considered right for
> 'qemu-img check'. Also, so far this information only exists for QED
> images, which makes it rather unlikely to be relied on. I think
> modifying it is okay (if it wasn't, we would even have to think twice
> about adding new fields).

Adding the JSON output will go a long way towards giving us freedom to
rework the human output however we'd like.  But even if we don't get
JSON added in time for 1.4, I don't think it is the end of the world for
this particular use case.
Kevin Wolf - Feb. 6, 2013, 2:58 p.m.
Am 06.02.2013 15:56, schrieb Eric Blake:
> On 02/06/2013 06:45 AM, Kevin Wolf wrote:
>>>> Can you add a space before the '=' while touching this?
>>>
>>> Perhaps he can, but as this is user-visible output, and we don't yet
>>> have a way to force JSON output to get at the same information,
>>> arbitrarily changing the output format may break some existing user that
>>> is scraping the output.  It would be nice if we had access to this
>>> output information in a more stable format.
>>
>> Are you aware of any existing user that makes use of this information?
> 
> Not particularly, which is why I'm okay if we modify the output.  I was
> just making sure we were thinking about the issue.
> 
>>
>> We're going to add JSON output soon (possibly even before this series
>> goes in) and scraping the output was never considered right for
>> 'qemu-img check'. Also, so far this information only exists for QED
>> images, which makes it rather unlikely to be relied on. I think
>> modifying it is okay (if it wasn't, we would even have to think twice
>> about adding new fields).
> 
> Adding the JSON output will go a long way towards giving us freedom to
> rework the human output however we'd like.  But even if we don't get
> JSON added in time for 1.4, I don't think it is the end of the world for
> this particular use case.

Hard freeze for 1.4 was last week, so both this and the JSON output are
1.5 material.

Kevin

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 85d3740..167d65f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -389,6 +389,7 @@  static int img_check(int argc, char **argv)
     const char *filename, *fmt;
     BlockDriverState *bs;
     BdrvCheckResult result;
+    BlockFragInfo *bfi = &result.bfi;
     int fix = 0;
     int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
 
@@ -468,11 +469,12 @@  static int img_check(int argc, char **argv)
         }
     }
 
-    if (result.bfi.total_clusters != 0 && result.bfi.allocated_clusters != 0) {
-        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n",
-        result.bfi.allocated_clusters, result.bfi.total_clusters,
-        result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters,
-        result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters);
+    if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
+        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
+               "%0.2f%% fragmented\n",
+               bfi->allocated_clusters, bfi->total_clusters,
+               bfi->allocated_clusters * 100.0 / bfi->total_clusters,
+               bfi->fragmented_clusters * 100.0 / bfi->allocated_clusters);
     }
 
     bdrv_delete(bs);