[Qemu-devel] [PATCH] qemu-io: Fix 'map' output

[Qemu-devel] [PATCH] qemu-io: Fix 'map' output

[Kevin Wolf]

The output of the 'map' command in qemu-io used to directly resemble
bdrv_is_allocated() and could contain many lines for small chunks that
all have the same allocation status. After this patch, they will be
coalesced into a single output line for a large chunk.

As a side effect, the command gains some error handling.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 475a8bd..5e6680b 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1635,12 +1635,43 @@ static const cmdinfo_t alloc_cmd = {
     .oneline    = "checks if a sector is present in the file",
 };
 
+
+static int map_is_allocated(int64_t sector_num, int64_t nb_sectors, int64_t *pnum)
+{
+    int num, num_checked;
+    int ret, firstret;
+
+    num_checked = MIN(nb_sectors, INT_MAX);
+    ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
+    if (ret < 0) {
+        return ret;
+    }
+
+    firstret = ret;
+    *pnum = num;
+
+    while (nb_sectors > 0 && ret == firstret) {
+        sector_num += num;
+        nb_sectors -= num;
+
+        num_checked = MIN(nb_sectors, INT_MAX);
+        ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
+        if (ret == firstret) {
+            *pnum += num;
+        } else {
+            break;
+        }
+    }
+
+    return firstret;
+}
+
 static int map_f(int argc, char **argv)
 {
     int64_t offset;
     int64_t nb_sectors;
     char s1[64];
-    int num, num_checked;
+    int64_t num;
     int ret;
     const char *retstr;
 
@@ -1648,12 +1679,17 @@ static int map_f(int argc, char **argv)
     nb_sectors = bs->total_sectors;
 
     do {
-        num_checked = MIN(nb_sectors, INT_MAX);
-        ret = bdrv_is_allocated(bs, offset, num_checked, &num);
+        ret = map_is_allocated(offset, nb_sectors, &num);
+        if (ret < 0) {
+            error_report("Failed to get allocation status: %s", strerror(-ret));
+            return 0;
+        }
+
         retstr = ret ? "    allocated" : "not allocated";
         cvtstr(offset << 9ULL, s1, sizeof(s1));
-        printf("[% 24" PRId64 "] % 8d/% 8d sectors %s at offset %s (%d)\n",
-               offset << 9ULL, num, num_checked, retstr, s1, ret);
+        printf("[% 24" PRId64 "] % 8" PRId64 "/% 8" PRId64 " sectors %s "
+               "at offset %s (%d)\n",
+               offset << 9ULL, num, nb_sectors, retstr, s1, ret);
 
         offset += num;
         nb_sectors -= num;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qemu-io: Fix 'map' output

[Stefan Hajnoczi]

On Wed, May 15, 2013 at 01:47:12PM +0200, Kevin Wolf wrote:
> +static int map_is_allocated(int64_t sector_num, int64_t nb_sectors, int64_t *pnum)
> +{
> +    int num, num_checked;
> +    int ret, firstret;
> +
> +    num_checked = MIN(nb_sectors, INT_MAX);
> +    ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    firstret = ret;
> +    *pnum = num;
> +
> +    while (nb_sectors > 0 && ret == firstret) {
> +        sector_num += num;
> +        nb_sectors -= num;
> +
> +        num_checked = MIN(nb_sectors, INT_MAX);
> +        ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> +        if (ret == firstret) {
> +            *pnum += num;
> +        } else {
> +            break;
> +        }

The break makes && ret == firstret redundant above.  I suggest just
while (nb_sectors > 0) { ... } which is easier to read.

Also, if you respin the patch please tweak the commit message.
"Coalesce 'map' output" is more specific than "Fix 'map' output" -
unless this really fixes a bug which you didn't mention in the commit
description.

Re: [Qemu-devel] [PATCH] qemu-io: Fix 'map' output

[Kevin Wolf]

Am 16.05.2013 um 11:14 hat Stefan Hajnoczi geschrieben:
> On Wed, May 15, 2013 at 01:47:12PM +0200, Kevin Wolf wrote:
> > +static int map_is_allocated(int64_t sector_num, int64_t nb_sectors, int64_t *pnum)
> > +{
> > +    int num, num_checked;
> > +    int ret, firstret;
> > +
> > +    num_checked = MIN(nb_sectors, INT_MAX);
> > +    ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    firstret = ret;
> > +    *pnum = num;
> > +
> > +    while (nb_sectors > 0 && ret == firstret) {
> > +        sector_num += num;
> > +        nb_sectors -= num;
> > +
> > +        num_checked = MIN(nb_sectors, INT_MAX);
> > +        ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> > +        if (ret == firstret) {
> > +            *pnum += num;
> > +        } else {
> > +            break;
> > +        }
> 
> The break makes && ret == firstret redundant above.  I suggest just
> while (nb_sectors > 0) { ... } which is easier to read.

Okay. I wasn't sure which was better. Don't know though how it came that
I have both checks now...

> Also, if you respin the patch please tweak the commit message.
> "Coalesce 'map' output" is more specific than "Fix 'map' output" -
> unless this really fixes a bug which you didn't mention in the commit
> description.

I'll change the title. It makes different formats behave the same even
if they work in different granularities. I think QED was bitten by this
in qemu-iotests somwhere because it could give different results than
qcow2, possibly also dependent on timing. Maybe I should mention that as
well in the commit message.

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: Fix 'map' output

[Stefan Hajnoczi]

On Thu, May 16, 2013 at 11:24:01AM +0200, Kevin Wolf wrote:
> Am 16.05.2013 um 11:14 hat Stefan Hajnoczi geschrieben:
> > On Wed, May 15, 2013 at 01:47:12PM +0200, Kevin Wolf wrote:
> > > +static int map_is_allocated(int64_t sector_num, int64_t nb_sectors, int64_t *pnum)
> > > +{
> > > +    int num, num_checked;
> > > +    int ret, firstret;
> > > +
> > > +    num_checked = MIN(nb_sectors, INT_MAX);
> > > +    ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> > > +    if (ret < 0) {
> > > +        return ret;
> > > +    }
> > > +
> > > +    firstret = ret;
> > > +    *pnum = num;
> > > +
> > > +    while (nb_sectors > 0 && ret == firstret) {
> > > +        sector_num += num;
> > > +        nb_sectors -= num;
> > > +
> > > +        num_checked = MIN(nb_sectors, INT_MAX);
> > > +        ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> > > +        if (ret == firstret) {
> > > +            *pnum += num;
> > > +        } else {
> > > +            break;
> > > +        }
> > 
> > The break makes && ret == firstret redundant above.  I suggest just
> > while (nb_sectors > 0) { ... } which is easier to read.
> 
> Okay. I wasn't sure which was better. Don't know though how it came that
> I have both checks now...
> 
> > Also, if you respin the patch please tweak the commit message.
> > "Coalesce 'map' output" is more specific than "Fix 'map' output" -
> > unless this really fixes a bug which you didn't mention in the commit
> > description.
> 
> I'll change the title. It makes different formats behave the same even
> if they work in different granularities. I think QED was bitten by this
> in qemu-iotests somwhere because it could give different results than
> qcow2, possibly also dependent on timing. Maybe I should mention that as
> well in the commit message.

Yes, please.  I didn't think of that.