Patchwork [RFC] Add compare subcommand for qemu-img

login
register
mail settings
Submitter Miroslav Rezanina
Date Aug. 1, 2012, 10:03 a.m.
Message ID <490157254.6767622.1343815423950.JavaMail.root@redhat.com>
Download mbox | patch
Permalink /patch/174428/
State New
Headers show

Comments

Miroslav Rezanina - Aug. 1, 2012, 10:03 a.m.
This patch adds compare subcommand that compares two images. Compare has following criteria:
- only data part is compared
- unallocated sectors are not read
- in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest
- qemu-img returns:
   - 0 if images are identical
   - 1 if images differ
   - 2 on error

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

Patch:
--
Paolo Bonzini - Aug. 1, 2012, 10:22 a.m.
Il 01/08/2012 12:03, Miroslav Rezanina ha scritto:
> This patch adds compare subcommand that compares two images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest
> - qemu-img returns:
>    - 0 if images are identical
>    - 1 if images differ
>    - 2 on error
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> 
> Patch:
> --
> diff --git a/block.c b/block.c
> index b38940b..919f8e9 100644
> --- a/block.c
> +++ b/block.c
> @@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>  
>  typedef struct BdrvCoIsAllocatedData {
>      BlockDriverState *bs;
> +    BlockDriverState *base;
>      int64_t sector_num;
>      int nb_sectors;
>      int *pnum;
> @@ -2414,6 +2415,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> +/* Coroutine wrapper for bdrv_is_allocated_above() */
> +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
> +{
> +    BdrvCoIsAllocatedData *data = opaque;
> +    BlockDriverState *top = data->bs;
> +    BlockDriverState *base = data->base;
> +
> +    data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
> +                                           data->nb_sectors,data->pnum);
> +    data->done = true;
> +}
> +
> +/*
> + * Synchronous wrapper around bdrv_co_is_allocated_above().
> + *
> + * See bdrv_co_is_allocated_above() for details.
> + */
> +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> +                      int64_t sector_num, int nb_sectors, int *pnum)
> +{
> +    Coroutine *co;
> +    BdrvCoIsAllocatedData data = {
> +        .bs = top,
> +        .base = base,
> +        .sector_num = sector_num,
> +        .nb_sectors = nb_sectors,
> +        .pnum = pnum,
> +        .done = false,
> +    };
> +
> +    co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
> +    qemu_coroutine_enter(co, &data);
> +    while (!data.done) {
> +        qemu_aio_wait();
> +    }
> +    return data.ret;
> +}
> +
>  BlockInfoList *qmp_query_block(Error **errp)
>  {
>      BlockInfoList *head = NULL, *cur_item = NULL;
> diff --git a/block.h b/block.h
> index c89590d..6f39da9 100644
> --- a/block.h
> +++ b/block.h
> @@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
>  int bdrv_has_zero_init(BlockDriverState *bs);
>  int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>                        int *pnum);
> -
> +int bdrv_is_allocated_above(BlockDriverState* top, BlockDriverState *base,
> +                            int64_t sector_num, int nb_sectors, int *pnum);
>  void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
>                         BlockErrorAction on_write_error);
>  BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 39419a0..5b34896 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -27,6 +27,12 @@ STEXI
>  @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
>  ETEXI
>  
> +DEF("compare", img_compare,
> +    "compare [-f fmt] [-g fmt] [-p] filename1 filename2")
> +STEXI
> +@item compare [-f fmt] [-g fmt] [-p] filename1 filename2
> +ETEXI
> +
>  DEF("convert", img_convert,
>      "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
>  STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 80cfb9b..99a2f16 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -96,7 +96,9 @@ static void help(void)
>             "  '-a' applies a snapshot (revert disk to saved state)\n"
>             "  '-c' creates a snapshot\n"
>             "  '-d' deletes a snapshot\n"
> -           "  '-l' lists all snapshots in the given image\n";
> +           "  '-l' lists all snapshots in the given image\n"
> +           "Parameters to compare subcommand:\n"
> +           "  '-g' Second image format (in case it differs from first image)\n";
>  
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>  
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +
> +static int64_t sectors_to_process(int64_t total, int64_t from)
> +{
> +  int64_t rv = total - from;
> +
> +  if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS))
> +     return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> +
> +  return rv;
> +}
> +
> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occured
> + */
> +
> +static int img_compare(int argc, char **argv)
> +{
> +    const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
> +    BlockDriverState *bs1, *bs2;
> +    int64_t total_sectors1, total_sectors2;
> +    uint8_t *buf1 = NULL, *buf2 = NULL;
> +    int pnum1, pnum2;
> +    int allocated1,allocated2;
> +    int flags = BDRV_O_FLAGS;
> +    int progress=0,ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
> +    int64_t total_sectors;
> +    int64_t sector_num = 0;
> +    int64_t nb_sectors;
> +    int c, rv, pnum;
> +    uint64_t bs_sectors;
> +    uint64_t progress_base;
> +
> +
> +    for(;;) {
> +        c = getopt(argc, argv, "pf:g:");
> +        if (c == -1) {
> +            break;
> +        }
> +        switch(c) {
> +        case 'f':
> +            fmt1 = optarg;
> +            if (fmt2 == NULL)
> +                fmt2 = optarg;
> +            break;
> +        case 'g':
> +            fmt2 = optarg;
> +            break;

I can guess why you chose 'g', but perhaps 'F' for consistency with
qemu-img rebase?

Also, perhaps we can add a "strict" mode (-S) that would fail if the
images are of different size, and if a sector is allocated in one but
unallocated in the other?

And a "silent" or "quiet" mode (-s or -q, your choice) that prints
nothing, too.

> +        case 'p':
> +            progress = 1;
> +            break;
> +        }
> +    }
> +    if (optind >= argc) {
> +        help();
> +        goto out3;
> +    }
> +    filename1 = argv[optind++];
> +    filename2 = argv[optind++];
> +
> +    /* Initialize before goto out */
> +    qemu_progress_init(progress, 2.0);
> +
> +    bs1 = bdrv_new_open(filename1, fmt1, flags);
> +    if (!bs1) {
> +      error_report("Can't open file %s", filename1);
> +      ret = 2;
> +      goto out3;
> +    }
> +
> +    bs2 = bdrv_new_open(filename2, fmt2, flags);
> +    if (!bs2) {
> +      error_report("Can't open file %s:",filename2);
> +      ret = 2;
> +      goto out2;
> +    }
> +
> +    qemu_progress_print(0, 100);
> +
> +    buf1 = qemu_blockalign(bs1,IO_BUF_SIZE);
> +    buf2 = qemu_blockalign(bs2,IO_BUF_SIZE);
> +    bdrv_get_geometry(bs1,&bs_sectors);
> +    total_sectors1 = bs_sectors;

You can make total_sectors1/2 unsigned, and avoid the assignment.

> +    bdrv_get_geometry(bs2,&bs_sectors);
> +    total_sectors2 = bs_sectors;
> +    total_sectors = total_sectors1;
> +    progress_base = total_sectors;

over_sectors = MAX(total_sectors1, total_sectors2);
total_sector = MIN(total_sectors1, total_sectors2);
over_num = over_sectors - total_sectors;

> +    if (total_sectors1 != total_sectors2) {

Using MAX/MIN as above, you can move this part to after you checked the
common part of the image, so that images are read sequentially.

You can still place the test here in strict mode.  With the assignments
given above, you can do the test simply like this:

   if (over_num > 0) {
   }

> +        BlockDriverState *bsover;
> +        int64_t over_sectors, over_num;
> +        const char *filename_over;
> +
> +        if (total_sectors1 > total_sectors2) {
> +            total_sectors = total_sectors2;
> +            over_sectors = total_sectors1;
> +            over_num = total_sectors2;
> +            bsover = bs1;
> +            filename_over = filename1;

Only bsover/filename_over are needed of course.

> +        } else {
> +            total_sectors = total_sectors1;
> +            over_sectors = total_sectors2;
> +            over_num = total_sectors1;
> +            bsover = bs1;

bsover = bs2;

> +            filename_over = filename2;

Again, of course only bsover/filename_over are needed with the changes
suggested above.

> +        }
> +
> +        progress_base = over_sectors;
> +
> +        for (;;) {
> +            if ((nb_sectors = sectors_to_process(over_sectors,over_num)) <= 0)
> +              break;
> +
> +            rv = bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum);
> +            nb_sectors = pnum;
> +            if (rv) {
> +               rv = bdrv_read(bsover, over_num, buf1, nb_sectors);
> +               if (rv < 0) {
> +                   error_report("error while reading sector %" PRId64 " of %s:"
> +                                " %s",over_num, filename_over, strerror(-rv));
> +                   ret = 2;
> +                   goto out;
> +               }
> +               rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> +               if (rv || pnum != nb_sectors) {
> +                   ret = 1;
> +                   printf("Images are different - image size mismatch!\n");

The error is not too precise.  Let's print "content mismatch at sector
123456", and warn if the images are of different size, but we are still
exiting with code 0.

> +                   goto out;
> +               }
> +            }
> +            over_num += nb_sectors;
> +            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);

qemu_progress_print(nb_sectors, over_sectors);

> +        }
> +    }
> +
> +    for (;;) {
> +        if ((nb_sectors = sectors_to_process(total_sectors, sector_num)) <= 0)
> +            break;
> +        allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
> +                                             &pnum1);
> +        allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
> +                                             &pnum2);
> +        if (pnum1 < pnum2) {
> +            nb_sectors = pnum1;
> +        } else {
> +            nb_sectors = pnum2;
> +        }
> +
> +        if (allocated1 == allocated2) {
> +            if (allocated1) {
> +              rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
> +              if (rv < 0) {
> +                  ret = 2;
> +                  error_report("error while reading sector %" PRId64 " of %s:"
> +                               " %s", sector_num, filename1, strerror(-rv));
> +                  goto out;
> +              }
> +              rv = bdrv_read(bs2,sector_num,buf2, nb_sectors);
> +              if (rv < 0) {
> +                  ret = 2;
> +                  error_report("error while reading sector %" PRId64 " of %s:"
> +                               " %s", sector_num, filename2, strerror(-rv));
> +                  goto out;
> +              }
> +              rv = compare_sectors(buf1,buf2,nb_sectors,&pnum);
> +              if (rv || pnum != nb_sectors) {

No need to check pnum != nb_sectors.

> +                  ret = 1;
> +                  printf("Images are different - content mismatch!\n");

Please print the sector number here.

> +                  goto out;
> +              }
> +            }
> +        } else {
> +           BlockDriverState *bstmp;
> +           const char* filenametmp;
> +           if (allocated1) {
> +               bstmp = bs1;
> +               filenametmp = filename1;
> +           } else {
> +               bstmp = bs2;
> +               filenametmp = filename2;
> +           }
> +           rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors);
> +           if (rv < 0) {
> +               ret = 2;
> +               error_report("error while reading sector %" PRId64 " of %s: %s",
> +                               sector_num, filenametmp, strerror(-rv));
> +               goto out;
> +           }
> +           rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> +           if (rv || pnum != nb_sectors) {
> +               ret = 1;
> +               printf("Images are different - content mismatch!\n");

Please print the sector number here.

> +               goto out;
> +           }
> +        }
> +        sector_num += nb_sectors;
> +        qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);

qemu_progress_print(nb_sectors, over_sectors);

Perhaps larger_sectors is a better name than over_sectors (and
larger_only_sectors instead of over_num? but I like this one a bit less...).

> +    }
> +    printf("Images are identical.\n");
> +
> +out:
> +    bdrv_delete(bs2);
> +    qemu_vfree(buf1);
> +    qemu_vfree(buf2);
> +out2:
> +    bdrv_delete(bs1);
> +out3:
> +    qemu_progress_end();
> +    return ret;
> +}
> +
>  static int img_convert(int argc, char **argv)
>  {
>      int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
> 

Only cosmetic problems overall; looks pretty good!

Paolo
Miroslav Rezanina - Aug. 1, 2012, 10:44 a.m.
----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org
> Sent: Wednesday, August 1, 2012 12:22:50 PM
> Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img
> 
> Il 01/08/2012 12:03, Miroslav Rezanina ha scritto:
> > This patch adds compare subcommand that compares two images.
> > Compare has following criteria:
> > - only data part is compared
> > - unallocated sectors are not read
> > - in case of different image size, exceeding part of bigger disk
> > has to be zeroed/unallocated to compare rest
> > - qemu-img returns:
> >    - 0 if images are identical
> >    - 1 if images differ
> >    - 2 on error
> > 
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > 
> > Patch:
> > --
> > diff --git a/block.c b/block.c
> > index b38940b..919f8e9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
> >  
> >  typedef struct BdrvCoIsAllocatedData {
> >      BlockDriverState *bs;
> > +    BlockDriverState *base;
> >      int64_t sector_num;
> >      int nb_sectors;
> >      int *pnum;
> > @@ -2414,6 +2415,44 @@ int coroutine_fn
> > bdrv_co_is_allocated_above(BlockDriverState *top,
> >      return 0;
> >  }
> >  
> > +/* Coroutine wrapper for bdrv_is_allocated_above() */
> > +static void coroutine_fn bdrv_is_allocated_above_co_entry(void
> > *opaque)
> > +{
> > +    BdrvCoIsAllocatedData *data = opaque;
> > +    BlockDriverState *top = data->bs;
> > +    BlockDriverState *base = data->base;
> > +
> > +    data->ret = bdrv_co_is_allocated_above(top, base,
> > data->sector_num,
> > +
> >                                           data->nb_sectors,data->pnum);
> > +    data->done = true;
> > +}
> > +
> > +/*
> > + * Synchronous wrapper around bdrv_co_is_allocated_above().
> > + *
> > + * See bdrv_co_is_allocated_above() for details.
> > + */
> > +int bdrv_is_allocated_above(BlockDriverState *top,
> > BlockDriverState *base,
> > +                      int64_t sector_num, int nb_sectors, int
> > *pnum)
> > +{
> > +    Coroutine *co;
> > +    BdrvCoIsAllocatedData data = {
> > +        .bs = top,
> > +        .base = base,
> > +        .sector_num = sector_num,
> > +        .nb_sectors = nb_sectors,
> > +        .pnum = pnum,
> > +        .done = false,
> > +    };
> > +
> > +    co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
> > +    qemu_coroutine_enter(co, &data);
> > +    while (!data.done) {
> > +        qemu_aio_wait();
> > +    }
> > +    return data.ret;
> > +}
> > +
> >  BlockInfoList *qmp_query_block(Error **errp)
> >  {
> >      BlockInfoList *head = NULL, *cur_item = NULL;
> > diff --git a/block.h b/block.h
> > index c89590d..6f39da9 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs,
> > int64_t sector_num, int nb_sectors);
> >  int bdrv_has_zero_init(BlockDriverState *bs);
> >  int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
> >  int nb_sectors,
> >                        int *pnum);
> > -
> > +int bdrv_is_allocated_above(BlockDriverState* top,
> > BlockDriverState *base,
> > +                            int64_t sector_num, int nb_sectors,
> > int *pnum);
> >  void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction
> >  on_read_error,
> >                         BlockErrorAction on_write_error);
> >  BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int
> >  is_read);
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 39419a0..5b34896 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -27,6 +27,12 @@ STEXI
> >  @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
> >  ETEXI
> >  
> > +DEF("compare", img_compare,
> > +    "compare [-f fmt] [-g fmt] [-p] filename1 filename2")
> > +STEXI
> > +@item compare [-f fmt] [-g fmt] [-p] filename1 filename2
> > +ETEXI
> > +
> >  DEF("convert", img_convert,
> >      "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o
> >      options] [-s snapshot_name] [-S sparse_size] filename
> >      [filename2 [...]] output_filename")
> >  STEXI
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 80cfb9b..99a2f16 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -96,7 +96,9 @@ static void help(void)
> >             "  '-a' applies a snapshot (revert disk to saved
> >             state)\n"
> >             "  '-c' creates a snapshot\n"
> >             "  '-d' deletes a snapshot\n"
> > -           "  '-l' lists all snapshots in the given image\n";
> > +           "  '-l' lists all snapshots in the given image\n"
> > +           "Parameters to compare subcommand:\n"
> > +           "  '-g' Second image format (in case it differs from
> > first image)\n";
> >  
> >      printf("%s\nSupported formats:", help_msg);
> >      bdrv_iterate_format(format_print, NULL);
> > @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t
> > *buf1, const uint8_t *buf2, int n,
> >  
> >  #define IO_BUF_SIZE (2 * 1024 * 1024)
> >  
> > +/*
> > + * Get number of sectors that can be stored in IO buffer.
> > + */
> > +
> > +static int64_t sectors_to_process(int64_t total, int64_t from)
> > +{
> > +  int64_t rv = total - from;
> > +
> > +  if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS))
> > +     return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> > +
> > +  return rv;
> > +}
> > +
> > +/*
> > + * Compares two images. Exit codes:
> > + *
> > + * 0 - Images are identical
> > + * 1 - Images differ
> > + * 2 - Error occured
> > + */
> > +
> > +static int img_compare(int argc, char **argv)
> > +{
> > +    const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
> > +    BlockDriverState *bs1, *bs2;
> > +    int64_t total_sectors1, total_sectors2;
> > +    uint8_t *buf1 = NULL, *buf2 = NULL;
> > +    int pnum1, pnum2;
> > +    int allocated1,allocated2;
> > +    int flags = BDRV_O_FLAGS;
> > +    int progress=0,ret = 0; /* return value - 0 Ident, 1
> > Different, 2 Error */
> > +    int64_t total_sectors;
> > +    int64_t sector_num = 0;
> > +    int64_t nb_sectors;
> > +    int c, rv, pnum;
> > +    uint64_t bs_sectors;
> > +    uint64_t progress_base;
> > +
> > +
> > +    for(;;) {
> > +        c = getopt(argc, argv, "pf:g:");
> > +        if (c == -1) {
> > +            break;
> > +        }
> > +        switch(c) {
> > +        case 'f':
> > +            fmt1 = optarg;
> > +            if (fmt2 == NULL)
> > +                fmt2 = optarg;
> > +            break;
> > +        case 'g':
> > +            fmt2 = optarg;
> > +            break;
> 
> I can guess why you chose 'g', but perhaps 'F' for consistency with
> qemu-img rebase?
> 
> Also, perhaps we can add a "strict" mode (-S) that would fail if the
> images are of different size, and if a sector is allocated in one but
> unallocated in the other?
> 
> And a "silent" or "quiet" mode (-s or -q, your choice) that prints
> nothing, too.
> 

Good idea with additional modes. I'm not sure if strict should fail on 
allocated/unallocated - you can compare sparse/non-sparce when is this
highly probable.

> > +        case 'p':
> > +            progress = 1;
> > +            break;
> > +        }
> > +    }
> > +    if (optind >= argc) {
> > +        help();
> > +        goto out3;
> > +    }
> > +    filename1 = argv[optind++];
> > +    filename2 = argv[optind++];
> > +
> > +    /* Initialize before goto out */
> > +    qemu_progress_init(progress, 2.0);
> > +
> > +    bs1 = bdrv_new_open(filename1, fmt1, flags);
> > +    if (!bs1) {
> > +      error_report("Can't open file %s", filename1);
> > +      ret = 2;
> > +      goto out3;
> > +    }
> > +
> > +    bs2 = bdrv_new_open(filename2, fmt2, flags);
> > +    if (!bs2) {
> > +      error_report("Can't open file %s:",filename2);
> > +      ret = 2;
> > +      goto out2;
> > +    }
> > +
> > +    qemu_progress_print(0, 100);
> > +
> > +    buf1 = qemu_blockalign(bs1,IO_BUF_SIZE);
> > +    buf2 = qemu_blockalign(bs2,IO_BUF_SIZE);
> > +    bdrv_get_geometry(bs1,&bs_sectors);
> > +    total_sectors1 = bs_sectors;
> 
> You can make total_sectors1/2 unsigned, and avoid the assignment.
> 

This is using similar construct as img_convert I inspire in. I will
change this in v2.

> > +    bdrv_get_geometry(bs2,&bs_sectors);
> > +    total_sectors2 = bs_sectors;
> > +    total_sectors = total_sectors1;
> > +    progress_base = total_sectors;
> 
> over_sectors = MAX(total_sectors1, total_sectors2);
> total_sector = MIN(total_sectors1, total_sectors2);
> over_num = over_sectors - total_sectors;
> 
> > +    if (total_sectors1 != total_sectors2) {
> 
> Using MAX/MIN as above, you can move this part to after you checked
> the
> common part of the image, so that images are read sequentially.
> 
> You can still place the test here in strict mode.  With the
> assignments
> given above, you can do the test simply like this:
> 
>    if (over_num > 0) {
>    }
> 

I want to have this check prior common part check - we do not have
to check rest of the image if we know there's something in this area
of disk.

> > +        BlockDriverState *bsover;
> > +        int64_t over_sectors, over_num;
> > +        const char *filename_over;
> > +
> > +        if (total_sectors1 > total_sectors2) {
> > +            total_sectors = total_sectors2;
> > +            over_sectors = total_sectors1;
> > +            over_num = total_sectors2;
> > +            bsover = bs1;
> > +            filename_over = filename1;
> 
> Only bsover/filename_over are needed of course.
> 
> > +        } else {
> > +            total_sectors = total_sectors1;
> > +            over_sectors = total_sectors2;
> > +            over_num = total_sectors1;
> > +            bsover = bs1;
> 
> bsover = bs2;
> 
> > +            filename_over = filename2;
> 
> Again, of course only bsover/filename_over are needed with the
> changes
> suggested above.
> 
> > +        }
> > +
> > +        progress_base = over_sectors;
> > +
> > +        for (;;) {
> > +            if ((nb_sectors =
> > sectors_to_process(over_sectors,over_num)) <= 0)
> > +              break;
> > +
> > +            rv =
> > bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum);
> > +            nb_sectors = pnum;
> > +            if (rv) {
> > +               rv = bdrv_read(bsover, over_num, buf1, nb_sectors);
> > +               if (rv < 0) {
> > +                   error_report("error while reading sector %"
> > PRId64 " of %s:"
> > +                                " %s",over_num, filename_over,
> > strerror(-rv));
> > +                   ret = 2;
> > +                   goto out;
> > +               }
> > +               rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> > +               if (rv || pnum != nb_sectors) {
> > +                   ret = 1;
> > +                   printf("Images are different - image size
> > mismatch!\n");
> 
> The error is not too precise.  Let's print "content mismatch at
> sector
> 123456", and warn if the images are of different size, but we are
> still
> exiting with code 0.
> 

Ok

> > +                   goto out;
> > +               }
> > +            }
> > +            over_num += nb_sectors;
> > +            qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> 
> qemu_progress_print(nb_sectors, over_sectors);
> 
> > +        }
> > +    }
> > +
> > +    for (;;) {
> > +        if ((nb_sectors = sectors_to_process(total_sectors,
> > sector_num)) <= 0)
> > +            break;
> > +        allocated1 = bdrv_is_allocated_above(bs1, NULL,
> > sector_num, nb_sectors,
> > +                                             &pnum1);
> > +        allocated2 = bdrv_is_allocated_above(bs2, NULL,
> > sector_num, nb_sectors,
> > +                                             &pnum2);
> > +        if (pnum1 < pnum2) {
> > +            nb_sectors = pnum1;
> > +        } else {
> > +            nb_sectors = pnum2;
> > +        }
> > +
> > +        if (allocated1 == allocated2) {
> > +            if (allocated1) {
> > +              rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
> > +              if (rv < 0) {
> > +                  ret = 2;
> > +                  error_report("error while reading sector %"
> > PRId64 " of %s:"
> > +                               " %s", sector_num, filename1,
> > strerror(-rv));
> > +                  goto out;
> > +              }
> > +              rv = bdrv_read(bs2,sector_num,buf2, nb_sectors);
> > +              if (rv < 0) {
> > +                  ret = 2;
> > +                  error_report("error while reading sector %"
> > PRId64 " of %s:"
> > +                               " %s", sector_num, filename2,
> > strerror(-rv));
> > +                  goto out;
> > +              }
> > +              rv = compare_sectors(buf1,buf2,nb_sectors,&pnum);
> > +              if (rv || pnum != nb_sectors) {
> 
> No need to check pnum != nb_sectors.

We have to check this as we compare more than one sector. If the first
one will be same but any other will not we get 0 return value and expect
whole chunk to be same.

> 
> > +                  ret = 1;
> > +                  printf("Images are different - content
> > mismatch!\n");
> 
> Please print the sector number here.

Ok.

> 
> > +                  goto out;
> > +              }
> > +            }
> > +        } else {
> > +           BlockDriverState *bstmp;
> > +           const char* filenametmp;
> > +           if (allocated1) {
> > +               bstmp = bs1;
> > +               filenametmp = filename1;
> > +           } else {
> > +               bstmp = bs2;
> > +               filenametmp = filename2;
> > +           }
> > +           rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors);
> > +           if (rv < 0) {
> > +               ret = 2;
> > +               error_report("error while reading sector %" PRId64
> > " of %s: %s",
> > +                               sector_num, filenametmp,
> > strerror(-rv));
> > +               goto out;
> > +           }
> > +           rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> > +           if (rv || pnum != nb_sectors) {
> > +               ret = 1;
> > +               printf("Images are different - content
> > mismatch!\n");
> 
> Please print the sector number here.
>

Ok.
 
> > +               goto out;
> > +           }
> > +        }
> > +        sector_num += nb_sectors;
> > +        qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> 
> qemu_progress_print(nb_sectors, over_sectors);
> 
> Perhaps larger_sectors is a better name than over_sectors (and
> larger_only_sectors instead of over_num? but I like this one a bit
> less...).
> 
> > +    }
> > +    printf("Images are identical.\n");
> > +
> > +out:
> > +    bdrv_delete(bs2);
> > +    qemu_vfree(buf1);
> > +    qemu_vfree(buf2);
> > +out2:
> > +    bdrv_delete(bs1);
> > +out3:
> > +    qemu_progress_end();
> > +    return ret;
> > +}
> > +
> >  static int img_convert(int argc, char **argv)
> >  {
> >      int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
> >      cluster_sectors;
> > 
> 
> Only cosmetic problems overall; looks pretty good!
> 
> Paolo
> 

I will update patch as you commented and resend.

Mirek
Paolo Bonzini - Aug. 1, 2012, 10:52 a.m.
Il 01/08/2012 12:44, Miroslav Rezanina ha scritto:
>>
>> Also, perhaps we can add a "strict" mode (-S) that would fail if the
>> images are of different size, and if a sector is allocated in one but
>> unallocated in the other?
>>
>> And a "silent" or "quiet" mode (-s or -q, your choice) that prints
>> nothing, too.
>>
> 
> Good idea with additional modes. I'm not sure if strict should fail on 
> allocated/unallocated - you can compare sparse/non-sparce when is this
> highly probable.

For that use case you probably know in advance that the size is the
same, so you can use normal non-strict mode.

Comparing allocated/unallocated on the other hand is helpful with unit
tests.

>> You can make total_sectors1/2 unsigned, and avoid the assignment.
>> 
> This is using similar construct as img_convert I inspire in. I will
> change this in v2.

Oh, sorry then---consistency is better, please keep it as it is now.

>>> > > +    bdrv_get_geometry(bs2,&bs_sectors);
>>> > > +    total_sectors2 = bs_sectors;
>>> > > +    total_sectors = total_sectors1;
>>> > > +    progress_base = total_sectors;
>> > 
>> > over_sectors = MAX(total_sectors1, total_sectors2);
>> > total_sector = MIN(total_sectors1, total_sectors2);
>> > over_num = over_sectors - total_sectors;
>> > 
>>> > > +    if (total_sectors1 != total_sectors2) {
>> > 
>> > Using MAX/MIN as above, you can move this part to after you checked
>> > the
>> > common part of the image, so that images are read sequentially.
>> > 
>> > You can still place the test here in strict mode.  With the
>> > assignments
>> > given above, you can do the test simply like this:
>> > 
>> >    if (over_num > 0) {
>> >    }
>> > 
> I want to have this check prior common part check - we do not have
> to check rest of the image if we know there's something in this area
> of disk.

But if one of the two images is preallocated raw, or qcow2 with
preallocated metadata, or your filesystem does not support is_allocated
on raw images, then you're still reading all of the image just to check
against zeros.

Admittedly there is no single correct solution.  Hopefully other
reviewers will break the tie.

Paolo
Stefan Hajnoczi - Aug. 1, 2012, 12:22 p.m.
On Wed, Aug 1, 2012 at 11:03 AM, Miroslav Rezanina <mrezanin@redhat.com> wrote:
> This patch adds compare subcommand that compares two images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest
> - qemu-img returns:
>    - 0 if images are identical
>    - 1 if images differ
>    - 2 on error

Please add qemu-img compare documentation to qemu-img.texi.
> @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +
> +static int64_t sectors_to_process(int64_t total, int64_t from)
> +{
> +  int64_t rv = total - from;
> +
> +  if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS))
> +     return IO_BUF_SIZE >> BDRV_SECTOR_BITS;

Please use git show | scripts/checkpatch.pl - to check coding style.

Stefan
Eric Blake - Aug. 1, 2012, 1:21 p.m.
On 08/01/2012 04:03 AM, Miroslav Rezanina wrote:
> This patch adds compare subcommand that compares two images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest
> - qemu-img returns:
>    - 0 if images are identical
>    - 1 if images differ
>    - 2 on error
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> 

> +++ b/qemu-img.c
> @@ -96,7 +96,9 @@ static void help(void)
>             "  '-a' applies a snapshot (revert disk to saved state)\n"
>             "  '-c' creates a snapshot\n"
>             "  '-d' deletes a snapshot\n"
> -           "  '-l' lists all snapshots in the given image\n";
> +           "  '-l' lists all snapshots in the given image\n"
> +           "Parameters to compare subcommand:\n"
> +           "  '-g' Second image format (in case it differs from first image)\n";

As written, this sounds like:

No -f, no -g => probe both
-f, no -g => -f applies to both
no -f, -g => probe first, use -g for second
-f, -g => use given formats for both

Is that really what you meant, or do we actually get:

-f, no -g => -f applies to first, probe second

I think both interpretations could make sense, but I'd prefer having the
omission of -g imply probing the second file type regardless of the
presence or absence of -f, for consistency.
Paolo Bonzini - Aug. 1, 2012, 1:23 p.m.
Il 01/08/2012 15:21, Eric Blake ha scritto:
>> > +++ b/qemu-img.c
>> > @@ -96,7 +96,9 @@ static void help(void)
>> >             "  '-a' applies a snapshot (revert disk to saved state)\n"
>> >             "  '-c' creates a snapshot\n"
>> >             "  '-d' deletes a snapshot\n"
>> > -           "  '-l' lists all snapshots in the given image\n";
>> > +           "  '-l' lists all snapshots in the given image\n"
>> > +           "Parameters to compare subcommand:\n"
>> > +           "  '-g' Second image format (in case it differs from first image)\n";
> As written, this sounds like:
> 
> No -f, no -g => probe both
> -f, no -g => -f applies to both
> no -f, -g => probe first, use -g for second
> -f, -g => use given formats for both
> 
> Is that really what you meant, or do we actually get:
> 
> -f, no -g => -f applies to first, probe second
> 
> I think both interpretations could make sense, but I'd prefer having the
> omission of -g imply probing the second file type regardless of the
> presence or absence of -f, for consistency.

+1

Paolo
Miroslav Rezanina - Aug. 2, 2012, 5:19 a.m.
----- Original Message -----
> From: "Eric Blake" <eblake@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>
> Sent: Wednesday, August 1, 2012 3:21:03 PM
> Subject: Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
> 
> On 08/01/2012 04:03 AM, Miroslav Rezanina wrote:
> > This patch adds compare subcommand that compares two images.
> > Compare has following criteria:
> > - only data part is compared
> > - unallocated sectors are not read
> > - in case of different image size, exceeding part of bigger disk
> > has to be zeroed/unallocated to compare rest
> > - qemu-img returns:
> >    - 0 if images are identical
> >    - 1 if images differ
> >    - 2 on error
> > 
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > 
> 
> > +++ b/qemu-img.c
> > @@ -96,7 +96,9 @@ static void help(void)
> >             "  '-a' applies a snapshot (revert disk to saved
> >             state)\n"
> >             "  '-c' creates a snapshot\n"
> >             "  '-d' deletes a snapshot\n"
> > -           "  '-l' lists all snapshots in the given image\n";
> > +           "  '-l' lists all snapshots in the given image\n"
> > +           "Parameters to compare subcommand:\n"
> > +           "  '-g' Second image format (in case it differs from
> > first image)\n";
> 
> As written, this sounds like:
> 
> No -f, no -g => probe both
> -f, no -g => -f applies to both
> no -f, -g => probe first, use -g for second
> -f, -g => use given formats for both
> 
> Is that really what you meant, or do we actually get:
> 

Yes, this is what I meant.

> -f, no -g => -f applies to first, probe second
> 
> I think both interpretations could make sense, but I'd prefer having
> the
> omission of -g imply probing the second file type regardless of the
> presence or absence of -f, for consistency.
> 

I was evaluating both approach. For me only -f means 'Use this format for input'.
In case of compare input is equal to both files so the -f value will be used for
both of them. There's only one subcommand allowing to specify multiple input files - convert 
and it use -f value for all input commands. However, unlike the compare it does not allow
to specify different format for different files and has variable number of input files.

So I decided to use -f as specification for both files. I do not have problem with using
it for first image only but prefer this handling. I can switch handling in next version in case
of more votes for it.

> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

Regards,
Miroslav Rezanina
Miroslav Rezanina - Aug. 2, 2012, 10:06 a.m.
----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org
> Sent: Wednesday, August 1, 2012 12:22:50 PM
> Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img
> 
> > +                   goto out;
> > +               }
> > +            }
> > +            over_num += nb_sectors;
> > +            qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> 
> qemu_progress_print(nb_sectors, over_sectors);
> 
> 
> Paolo
> 

This won't work. qemu_progress_print takes either (current_progress,0) or
(progress_delta,progress_part) where all values are in percents - values in
range (0,100). If we pass (nb_sectors,over_sectors) we get 100% thanks to the 
formula:

nb_sectors / 100 * over_sectors

Mirek
Paolo Bonzini - Aug. 2, 2012, 11:11 a.m.
Il 02/08/2012 12:06, Miroslav Rezanina ha scritto:
>>> > > +            qemu_progress_print(((float) nb_sectors /
>>> > > progress_base)*100, 100);
>> > 
>> > qemu_progress_print(nb_sectors, over_sectors);
>> > 
>> > 
>> > Paolo
>> > 
> This won't work. qemu_progress_print takes either (current_progress,0) or
> (progress_delta,progress_part) where all values are in percents - values in
> range (0,100). If we pass (nb_sectors,over_sectors) we get 100% thanks to the 
> formula:
> 
> nb_sectors / 100 * over_sectors

Uff, I admit I never understood qemu_progress_print. :)

Paolo

Patch

diff --git a/block.c b/block.c
index b38940b..919f8e9 100644
--- a/block.c
+++ b/block.c
@@ -2284,6 +2284,7 @@  int bdrv_has_zero_init(BlockDriverState *bs)
 
 typedef struct BdrvCoIsAllocatedData {
     BlockDriverState *bs;
+    BlockDriverState *base;
     int64_t sector_num;
     int nb_sectors;
     int *pnum;
@@ -2414,6 +2415,44 @@  int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
+/* Coroutine wrapper for bdrv_is_allocated_above() */
+static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
+{
+    BdrvCoIsAllocatedData *data = opaque;
+    BlockDriverState *top = data->bs;
+    BlockDriverState *base = data->base;
+
+    data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
+                                           data->nb_sectors,data->pnum);
+    data->done = true;
+}
+
+/*
+ * Synchronous wrapper around bdrv_co_is_allocated_above().
+ *
+ * See bdrv_co_is_allocated_above() for details.
+ */
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+                      int64_t sector_num, int nb_sectors, int *pnum)
+{
+    Coroutine *co;
+    BdrvCoIsAllocatedData data = {
+        .bs = top,
+        .base = base,
+        .sector_num = sector_num,
+        .nb_sectors = nb_sectors,
+        .pnum = pnum,
+        .done = false,
+    };
+
+    co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
+    qemu_coroutine_enter(co, &data);
+    while (!data.done) {
+        qemu_aio_wait();
+    }
+    return data.ret;
+}
+
 BlockInfoList *qmp_query_block(Error **errp)
 {
     BlockInfoList *head = NULL, *cur_item = NULL;
diff --git a/block.h b/block.h
index c89590d..6f39da9 100644
--- a/block.h
+++ b/block.h
@@ -256,7 +256,8 @@  int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                       int *pnum);
-
+int bdrv_is_allocated_above(BlockDriverState* top, BlockDriverState *base,
+                            int64_t sector_num, int nb_sectors, int *pnum);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
 BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 39419a0..5b34896 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -27,6 +27,12 @@  STEXI
 @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
 ETEXI
 
+DEF("compare", img_compare,
+    "compare [-f fmt] [-g fmt] [-p] filename1 filename2")
+STEXI
+@item compare [-f fmt] [-g fmt] [-p] filename1 filename2
+ETEXI
+
 DEF("convert", img_convert,
     "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index 80cfb9b..99a2f16 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -96,7 +96,9 @@  static void help(void)
            "  '-a' applies a snapshot (revert disk to saved state)\n"
            "  '-c' creates a snapshot\n"
            "  '-d' deletes a snapshot\n"
-           "  '-l' lists all snapshots in the given image\n";
+           "  '-l' lists all snapshots in the given image\n"
+           "Parameters to compare subcommand:\n"
+           "  '-g' Second image format (in case it differs from first image)\n";
 
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);
@@ -652,6 +654,223 @@  static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
 
 #define IO_BUF_SIZE (2 * 1024 * 1024)
 
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+  int64_t rv = total - from;
+
+  if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS))
+     return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
+
+  return rv;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occured
+ */
+
+static int img_compare(int argc, char **argv)
+{
+    const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+    BlockDriverState *bs1, *bs2;
+    int64_t total_sectors1, total_sectors2;
+    uint8_t *buf1 = NULL, *buf2 = NULL;
+    int pnum1, pnum2;
+    int allocated1,allocated2;
+    int flags = BDRV_O_FLAGS;
+    int progress=0,ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+    int64_t total_sectors;
+    int64_t sector_num = 0;
+    int64_t nb_sectors;
+    int c, rv, pnum;
+    uint64_t bs_sectors;
+    uint64_t progress_base;
+
+
+    for(;;) {
+        c = getopt(argc, argv, "pf:g:");
+        if (c == -1) {
+            break;
+        }
+        switch(c) {
+        case 'f':
+            fmt1 = optarg;
+            if (fmt2 == NULL)
+                fmt2 = optarg;
+            break;
+        case 'g':
+            fmt2 = optarg;
+            break;
+        case 'p':
+            progress = 1;
+            break;
+        }
+    }
+    if (optind >= argc) {
+        help();
+        goto out3;
+    }
+    filename1 = argv[optind++];
+    filename2 = argv[optind++];
+
+    /* Initialize before goto out */
+    qemu_progress_init(progress, 2.0);
+
+    bs1 = bdrv_new_open(filename1, fmt1, flags);
+    if (!bs1) {
+      error_report("Can't open file %s", filename1);
+      ret = 2;
+      goto out3;
+    }
+
+    bs2 = bdrv_new_open(filename2, fmt2, flags);
+    if (!bs2) {
+      error_report("Can't open file %s:",filename2);
+      ret = 2;
+      goto out2;
+    }
+
+    qemu_progress_print(0, 100);
+
+    buf1 = qemu_blockalign(bs1,IO_BUF_SIZE);
+    buf2 = qemu_blockalign(bs2,IO_BUF_SIZE);
+    bdrv_get_geometry(bs1,&bs_sectors);
+    total_sectors1 = bs_sectors;
+    bdrv_get_geometry(bs2,&bs_sectors);
+    total_sectors2 = bs_sectors;
+    total_sectors = total_sectors1;
+    progress_base = total_sectors;
+
+    if (total_sectors1 != total_sectors2) {
+        BlockDriverState *bsover;
+        int64_t over_sectors, over_num;
+        const char *filename_over;
+
+        if (total_sectors1 > total_sectors2) {
+            total_sectors = total_sectors2;
+            over_sectors = total_sectors1;
+            over_num = total_sectors2;
+            bsover = bs1;
+            filename_over = filename1;
+        } else {
+            total_sectors = total_sectors1;
+            over_sectors = total_sectors2;
+            over_num = total_sectors1;
+            bsover = bs1;
+            filename_over = filename2;
+        }
+
+        progress_base = over_sectors;
+
+        for (;;) {
+            if ((nb_sectors = sectors_to_process(over_sectors,over_num)) <= 0)
+              break;
+
+            rv = bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum);
+            nb_sectors = pnum;
+            if (rv) {
+               rv = bdrv_read(bsover, over_num, buf1, nb_sectors);
+               if (rv < 0) {
+                   error_report("error while reading sector %" PRId64 " of %s:"
+                                " %s",over_num, filename_over, strerror(-rv));
+                   ret = 2;
+                   goto out;
+               }
+               rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+               if (rv || pnum != nb_sectors) {
+                   ret = 1;
+                   printf("Images are different - image size mismatch!\n");
+                   goto out;
+               }
+            }
+            over_num += nb_sectors;
+            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+        }
+    }
+
+    for (;;) {
+        if ((nb_sectors = sectors_to_process(total_sectors, sector_num)) <= 0)
+            break;
+        allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+                                             &pnum1);
+        allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+                                             &pnum2);
+        if (pnum1 < pnum2) {
+            nb_sectors = pnum1;
+        } else {
+            nb_sectors = pnum2;
+        }
+
+        if (allocated1 == allocated2) {
+            if (allocated1) {
+              rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+              if (rv < 0) {
+                  ret = 2;
+                  error_report("error while reading sector %" PRId64 " of %s:"
+                               " %s", sector_num, filename1, strerror(-rv));
+                  goto out;
+              }
+              rv = bdrv_read(bs2,sector_num,buf2, nb_sectors);
+              if (rv < 0) {
+                  ret = 2;
+                  error_report("error while reading sector %" PRId64 " of %s:"
+                               " %s", sector_num, filename2, strerror(-rv));
+                  goto out;
+              }
+              rv = compare_sectors(buf1,buf2,nb_sectors,&pnum);
+              if (rv || pnum != nb_sectors) {
+                  ret = 1;
+                  printf("Images are different - content mismatch!\n");
+                  goto out;
+              }
+            }
+        } else {
+           BlockDriverState *bstmp;
+           const char* filenametmp;
+           if (allocated1) {
+               bstmp = bs1;
+               filenametmp = filename1;
+           } else {
+               bstmp = bs2;
+               filenametmp = filename2;
+           }
+           rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors);
+           if (rv < 0) {
+               ret = 2;
+               error_report("error while reading sector %" PRId64 " of %s: %s",
+                               sector_num, filenametmp, strerror(-rv));
+               goto out;
+           }
+           rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+           if (rv || pnum != nb_sectors) {
+               ret = 1;
+               printf("Images are different - content mismatch!\n");
+               goto out;
+           }
+        }
+        sector_num += nb_sectors;
+        qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+    }
+    printf("Images are identical.\n");
+
+out:
+    bdrv_delete(bs2);
+    qemu_vfree(buf1);
+    qemu_vfree(buf2);
+out2:
+    bdrv_delete(bs1);
+out3:
+    qemu_progress_end();
+    return ret;
+}
+
 static int img_convert(int argc, char **argv)
 {
     int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;