| Message ID | 1368618432-4431-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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;
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(-)