| Message ID | 1360148318-26988-4-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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
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);
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(-)