Patchwork [v6] Add compare subcommand for qemu-img

login
register
mail settings
Submitter Miroslav Rezanina
Date Dec. 6, 2012, 12:24 p.m.
Message ID <1340958014.11395601.1354796644818.JavaMail.root@redhat.com>
Download mbox | patch
Permalink /patch/204214/
State New
Headers show

Comments

Miroslav Rezanina - Dec. 6, 2012, 12:24 p.m.
This is second version of  patch adding 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

v6:
 - added handling -?, -h options for compare subcommand

v5 (only minor changes):
 - removed redundant comment
 - removed dead code (goto after help())
 - set final total_sectors on first assignment

v4:
 - Fixed various typos
 - Added functions for empty sector check and sector-to-bytes offset
 conversion
 - Fixed command-line parameters processing

v3:
 - options -f/-F are orthogonal
 - documentation updated to new syntax and behavior
 - used byte offset instead of sector number for output
 
v2:
 - changed option for second image format to -F
 - changed handling of -f and -F [1]
 - added strict mode (-s)
 - added quiet mode (-q)
 - improved output messages [2]
 - rename variables for larger image handling
 - added man page content

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
Stefan Hajnoczi - Dec. 11, 2012, 9:16 a.m.
On Thu, Dec 06, 2012 at 07:24:04AM -0500, Miroslav Rezanina wrote:
> This is second version of  patch adding 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
> 
> v6:
>  - added handling -?, -h options for compare subcommand
> 
> v5 (only minor changes):
>  - removed redundant comment
>  - removed dead code (goto after help())
>  - set final total_sectors on first assignment
> 
> v4:
>  - Fixed various typos
>  - Added functions for empty sector check and sector-to-bytes offset
>  conversion
>  - Fixed command-line parameters processing
> 
> v3:
>  - options -f/-F are orthogonal
>  - documentation updated to new syntax and behavior
>  - used byte offset instead of sector number for output
>  
> v2:
>  - changed option for second image format to -F
>  - changed handling of -f and -F [1]
>  - added strict mode (-s)
>  - added quiet mode (-q)
>  - improved output messages [2]
>  - rename variables for larger image handling
>  - added man page content
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf - Dec. 11, 2012, 12:27 p.m.
Am 06.12.2012 13:24, schrieb Miroslav Rezanina:
> This is second version of  patch adding 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
> 
> v6:
>  - added handling -?, -h options for compare subcommand
> 
> v5 (only minor changes):
>  - removed redundant comment
>  - removed dead code (goto after help())
>  - set final total_sectors on first assignment
> 
> v4:
>  - Fixed various typos
>  - Added functions for empty sector check and sector-to-bytes offset
>  conversion
>  - Fixed command-line parameters processing
> 
> v3:
>  - options -f/-F are orthogonal
>  - documentation updated to new syntax and behavior
>  - used byte offset instead of sector number for output
>  
> v2:
>  - changed option for second image format to -F
>  - changed handling of -f and -F [1]
>  - added strict mode (-s)
>  - added quiet mode (-q)
>  - improved output messages [2]
>  - rename variables for larger image handling
>  - added man page content
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

Please move the changelog below a "---" line so that git am ignores it
for the final commit message.

> diff --git a/block.c b/block.c
> index 854ebd6..fdc8c45 100644
> --- a/block.c
> +++ b/block.c
> @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>  
>  typedef struct BdrvCoIsAllocatedData {
>      BlockDriverState *bs;
> +    BlockDriverState *base;
>      int64_t sector_num;
>      int nb_sectors;
>      int *pnum;
> @@ -2828,6 +2829,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;
> +}
> +
>  BlockInfo *bdrv_query_info(BlockDriverState *bs)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/block.h b/block.h
> index 722c620..2cb8d71 100644
> --- a/block.h
> +++ b/block.h
> @@ -278,6 +278,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, BlockdevOnError on_read_error,
>                         BlockdevOnError on_write_error);

This first part looks good. I think it could be a separate patch, however.

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index a181363..c076085 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] [-F fmt] [-p] [-q] [-s] filename1 filename2")
> +STEXI
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{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 e29e01b..09abb3a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,13 @@ 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"
> +           "\n"
> +           "Parameters to compare subcommand:\n"
> +           "  '-f' First image format\n"
> +           "  '-F' Second image format\n"
> +           "  '-q' Quiet mode - do not print any output (except errors)\n"
> +           "  '-s' Strict mode - fail on different image size or sector allocation\n";

Let's put -q in the section for common options. It's currently only in
compare, but it definitely makes sense for other subcommands.

>  
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>  
> +static int64_t sectors_to_bytes(int64_t sectors)
> +{
> +    return sectors << BDRV_SECTOR_BITS;
> +}
> +
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +static int64_t sectors_to_process(int64_t total, int64_t from)

According to the comment it would always return IO_BUF_SIZE, so I would
suggest to rephrase it.

> +{
> +    int64_t rv = total - from;
> +
> +    if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> +        return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> +    }
> +
> +    return rv;
> +}

What does 'rv' mean? Return value? If so, the common name at least in
block layer code is 'ret'.

> +
> +/*
> + * Check if passed sectors are filled with 0.
> + *
> + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
> + * data and negative value on error.
> + */
> +static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
> +              const char *filename, uint8_t *buffer, int quiet)

Shouldn't pass the file name here. Also 'bool quiet'.

A function name is_zeroed() suggests that the result is a boolean, i.e.
0 means "not zeroed" and > 0 means "zeroed".

> +{
> +    int pnum, rv = 0;
> +    rv = bdrv_read(bs, sect_num, buffer, sect_count);
> +    if (rv < 0) {
> +        error_report("Error while reading offset %" PRId64 " of %s: %s",
> +                     sectors_to_bytes(sect_num), filename, strerror(-rv));

If you want to include the file name, you can use the loc_*() functions.

> +        return rv;
> +    }
> +    rv = is_allocated_sectors(buffer, sect_count, &pnum);
> +    if (rv || pnum != sect_count) {
> +        if (!quiet) {
> +            printf("Content mismatch at offset %" PRId64 "!\n",
> +                   sectors_to_bytes(rv ? sect_num : sect_num + pnum));

Sure that this error message is at the right layer? The function header
comment doesn't say anything about the situation in which the function
must be used.

> +        }
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occurred
> + */
> +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;

This is never changed. You can use the constant directly.

> +    int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
> +    int progress = 0, quiet = 0, strict = 0;

bool

> +    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, "hpf:F:sq");
> +        if (c == -1) {
> +            break;
> +        }
> +        switch (c) {
> +        case '?':
> +        case 'h':
> +            help();
> +            break;
> +        case 'f':
> +            fmt1 = optarg;
> +            break;
> +        case 'F':
> +            fmt2 = optarg;
> +            break;
> +        case 'p':
> +            progress = 1;
> +            break;
> +        case 'q':
> +            quiet = 1;
> +            break;
> +        case 's':
> +            strict = 1;
> +            break;
> +        }
> +    }
> +
> +    /* Progress is not shown in Quiet mode */
> +    if (quiet) {
> +        progress = 0;
> +    }
> +
> +    if (optind > argc - 2) {
> +        help();
> +    }
> +    filename1 = argv[optind++];
> +    filename2 = argv[optind++];
> +
> +    /* Initialize before goto out */
> +    qemu_progress_init(progress, 2.0);
> +
> +    bs1 = bdrv_new_open(filename1, fmt1, flags, true);
> +    if (!bs1) {
> +        error_report("Can't open file %s", filename1);
> +        ret = 2;
> +        goto out3;
> +    }
> +
> +    bs2 = bdrv_new_open(filename2, fmt2, flags, true);
> +    if (!bs2) {
> +        error_report("Can't open file %s", filename2);
> +        ret = 2;
> +        goto out2;
> +    }
> +
> +    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 = MIN(total_sectors1, total_sectors2);
> +    progress_base = total_sectors;
> +
> +    qemu_progress_print(0, 100);
> +
> +    if (total_sectors1 != total_sectors2) {
> +        BlockDriverState *bsover;
> +        int64_t lo_total_sectors, lo_sector_num;
> +        const char *filename_over;
> +
> +        if (strict) {
> +            ret = 1;
> +            if (!quiet) {
> +                printf("Strict mode: Image size mismatch!\n");
> +            }
> +            goto out;
> +        } else {
> +            error_report("Image size mismatch!");
> +        }
> +
> +        if (total_sectors1 > total_sectors2) {
> +            lo_total_sectors = total_sectors1;
> +            lo_sector_num = total_sectors2;
> +            bsover = bs1;
> +            filename_over = filename1;
> +        } else {
> +            lo_total_sectors = total_sectors2;
> +            lo_sector_num = total_sectors1;
> +            bsover = bs2;
> +            filename_over = filename2;
> +        }
> +
> +        progress_base = lo_total_sectors;
> +
> +        for (;;) {
> +            nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
> +            if (nb_sectors <= 0) {
> +                break;
> +            }
> +            rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num,
> +                                         nb_sectors, &pnum);
> +            nb_sectors = pnum;
> +            if (rv) {
> +                rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
> +                               filename_over, buf1, quiet);
> +                if (rv) {
> +                    if (rv < 0) {
> +                        ret = 2;
> +                    }
> +                    goto out;
> +                }
> +            }
> +            lo_sector_num += nb_sectors;
> +            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
> +        }
> +    }

Hm, nb_sectors is the number of sectors processed in one loop iteration,
isn't it? How can you calculate a meaningful progress from it then? And
even if it did work, wouldn't the progress bar jump backwards when it
starts comparing the part that exists in both images?

And didn't we decide that qemu-img compare should always return the
lowest offset at which the images differ?

> +
> +
> +    for (;;) {
> +        nb_sectors = sectors_to_process(total_sectors, sector_num);
> +        if (nb_sectors <= 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;
> +        }

nb_sectors = MIN(pnum1, pnum2);

> @@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
>  
>  Commit the changes recorded in @var{filename} in its base image.
>  
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
> +
> +Check if two images contains same content. You can compare images with

s/contains/contain the/

Or maybe even "have" the same content. Containing content is kind of
redundant.

> +different format or settings.
> +
> +Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.

s/Format/The format/

> +
> +By default, compare evaluates as identical images with different size where
> +bigger image contains only unallocated and/or zeroed sectors in area above
> +second image size.

Hm, let me try and rephrase this one...

"By default, images with different size are considered identical if the
larger image contains only unallocated and/or zeroed sectors in the area
after the end of the other image."

> In addition, if any sector is not allocated in one image
> +and contains only zero bytes in second, it is evaluated as equal.

s/in second/in the second one/

> You can use
> +Strict mode by specifying @var{-s} option. When compare runs in Strict mode,

_the_ -s option

> +it fails in case image size differs or sector is allocated in one image and

_a_ sector

> +is not allocated in second.

the second one.

> +
> +By default, compare prints out result message. This message displays

_a_ result message

> +information that both images are same or position of first different byte.

_the_ position of _the_ first different byte

> +In addition, result message can report different image size in case Strict
> +mode is used.

_the_ result message can report an image size difference

> +In case you want to suppress any non-error output, you can use Quiet mode by
> +specifying @var{-q} option.

_the_ -q option

Kevin
Miroslav Rezanina - Dec. 11, 2012, 1:09 p.m.
Hi Kevin,
thanks for review, comments inline.

----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent: Tuesday, December 11, 2012 1:27:45 PM
> Subject: Re: [PATCH v6] Add compare subcommand for qemu-img
> 
> Am 06.12.2012 13:24, schrieb Miroslav Rezanina:
> > This is second version of  patch adding 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
> > 
> > v6:
> >  - added handling -?, -h options for compare subcommand
> > 
> > v5 (only minor changes):
> >  - removed redundant comment
> >  - removed dead code (goto after help())
> >  - set final total_sectors on first assignment
> > 
> > v4:
> >  - Fixed various typos
> >  - Added functions for empty sector check and sector-to-bytes
> >  offset
> >  conversion
> >  - Fixed command-line parameters processing
> > 
> > v3:
> >  - options -f/-F are orthogonal
> >  - documentation updated to new syntax and behavior
> >  - used byte offset instead of sector number for output
> >  
> > v2:
> >  - changed option for second image format to -F
> >  - changed handling of -f and -F [1]
> >  - added strict mode (-s)
> >  - added quiet mode (-q)
> >  - improved output messages [2]
> >  - rename variables for larger image handling
> >  - added man page content
> > 
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> 
> Please move the changelog below a "---" line so that git am ignores
> it
> for the final commit message.
> 
Ok, next version probably be as multipart so this will go into the header mail.

> > diff --git a/block.c b/block.c
> > index 854ebd6..fdc8c45 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
> >  
> >  typedef struct BdrvCoIsAllocatedData {
> >      BlockDriverState *bs;
> > +    BlockDriverState *base;
> >      int64_t sector_num;
> >      int nb_sectors;
> >      int *pnum;
> > @@ -2828,6 +2829,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;
> > +}
> > +
> >  BlockInfo *bdrv_query_info(BlockDriverState *bs)
> >  {
> >      BlockInfo *info = g_malloc0(sizeof(*info));
> > diff --git a/block.h b/block.h
> > index 722c620..2cb8d71 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -278,6 +278,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, BlockdevOnError
> >  on_read_error,
> >                         BlockdevOnError on_write_error);
> 
> This first part looks good. I think it could be a separate patch,
> however.
> 

Agree with this, was lazy to split.

> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index a181363..c076085 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] [-F fmt] [-p] [-q] [-s] filename1
> > filename2")
> > +STEXI
> > +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s]
> > @var{filename1} @var{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 e29e01b..09abb3a 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -101,7 +101,13 @@ 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"
> > +           "\n"
> > +           "Parameters to compare subcommand:\n"
> > +           "  '-f' First image format\n"
> > +           "  '-F' Second image format\n"
> > +           "  '-q' Quiet mode - do not print any output (except
> > errors)\n"
> > +           "  '-s' Strict mode - fail on different image size or
> > sector allocation\n";
> 
> Let's put -q in the section for common options. It's currently only
> in
> compare, but it definitely makes sense for other subcommands.
> 

Ok, I can take care of this for other commands in next version (as one patch of the serie).

> >  
> >      printf("%s\nSupported formats:", help_msg);
> >      bdrv_iterate_format(format_print, NULL);
> > @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t
> > *buf1, const uint8_t *buf2, int n,
> >  
> >  #define IO_BUF_SIZE (2 * 1024 * 1024)
> >  
> > +static int64_t sectors_to_bytes(int64_t sectors)
> > +{
> > +    return sectors << BDRV_SECTOR_BITS;
> > +}
> > +
> > +/*
> > + * Get number of sectors that can be stored in IO buffer.
> > + */
> > +static int64_t sectors_to_process(int64_t total, int64_t from)
> 
> According to the comment it would always return IO_BUF_SIZE, so I
> would
> suggest to rephrase it.
> 

Right, can is not correctly used here.

> > +{
> > +    int64_t rv = total - from;
> > +
> > +    if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> > +        return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> > +    }
> > +
> > +    return rv;
> > +}
> 
> What does 'rv' mean? Return value? If so, the common name at least in
> block layer code is 'ret'.
> 

My fault, used to rv, fix it to ret in next version.

> > +
> > +/*
> > + * Check if passed sectors are filled with 0.
> > + *
> > + * Returns 0 in case sectors are filled with 0, 1 if sectors
> > contain non-zero
> > + * data and negative value on error.
> > + */
> > +static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int
> > sect_count,
> > +              const char *filename, uint8_t *buffer, int quiet)
> 
> Shouldn't pass the file name here. Also 'bool quiet'.
> 
> A function name is_zeroed() suggests that the result is a boolean,
> i.e.
> 0 means "not zeroed" and > 0 means "zeroed".
> 

Yeah, the function name is problem here. In fact it's purpose is to compare
passed block of data with block of zero bytes. It's common part for checking 
bytes in bigger image and for case one image has block allocated and second hasn't.
That's the reason quiet and filename are passed and error handling is done in the
function - to minimaze duplicate code. So the question is, if I should rename the
function or make it more general.

> > +{
> > +    int pnum, rv = 0;
> > +    rv = bdrv_read(bs, sect_num, buffer, sect_count);
> > +    if (rv < 0) {
> > +        error_report("Error while reading offset %" PRId64 " of
> > %s: %s",
> > +                     sectors_to_bytes(sect_num), filename,
> > strerror(-rv));
> 
> If you want to include the file name, you can use the loc_*()
> functions.
> 
> > +        return rv;
> > +    }
> > +    rv = is_allocated_sectors(buffer, sect_count, &pnum);
> > +    if (rv || pnum != sect_count) {
> > +        if (!quiet) {
> > +            printf("Content mismatch at offset %" PRId64 "!\n",
> > +                   sectors_to_bytes(rv ? sect_num : sect_num +
> > pnum));
> 
> Sure that this error message is at the right layer? The function
> header
> comment doesn't say anything about the situation in which the
> function
> must be used.
> 
> > +        }
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * Compares two images. Exit codes:
> > + *
> > + * 0 - Images are identical
> > + * 1 - Images differ
> > + * 2 - Error occurred
> > + */
> > +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;
> 
> This is never changed. You can use the constant directly.
> 
> > +    int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error
> > */
> > +    int progress = 0, quiet = 0, strict = 0;
> 
> bool
> 
> > +    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, "hpf:F:sq");
> > +        if (c == -1) {
> > +            break;
> > +        }
> > +        switch (c) {
> > +        case '?':
> > +        case 'h':
> > +            help();
> > +            break;
> > +        case 'f':
> > +            fmt1 = optarg;
> > +            break;
> > +        case 'F':
> > +            fmt2 = optarg;
> > +            break;
> > +        case 'p':
> > +            progress = 1;
> > +            break;
> > +        case 'q':
> > +            quiet = 1;
> > +            break;
> > +        case 's':
> > +            strict = 1;
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* Progress is not shown in Quiet mode */
> > +    if (quiet) {
> > +        progress = 0;
> > +    }
> > +
> > +    if (optind > argc - 2) {
> > +        help();
> > +    }
> > +    filename1 = argv[optind++];
> > +    filename2 = argv[optind++];
> > +
> > +    /* Initialize before goto out */
> > +    qemu_progress_init(progress, 2.0);
> > +
> > +    bs1 = bdrv_new_open(filename1, fmt1, flags, true);
> > +    if (!bs1) {
> > +        error_report("Can't open file %s", filename1);
> > +        ret = 2;
> > +        goto out3;
> > +    }
> > +
> > +    bs2 = bdrv_new_open(filename2, fmt2, flags, true);
> > +    if (!bs2) {
> > +        error_report("Can't open file %s", filename2);
> > +        ret = 2;
> > +        goto out2;
> > +    }
> > +
> > +    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 = MIN(total_sectors1, total_sectors2);
> > +    progress_base = total_sectors;
> > +
> > +    qemu_progress_print(0, 100);
> > +
> > +    if (total_sectors1 != total_sectors2) {
> > +        BlockDriverState *bsover;
> > +        int64_t lo_total_sectors, lo_sector_num;
> > +        const char *filename_over;
> > +
> > +        if (strict) {
> > +            ret = 1;
> > +            if (!quiet) {
> > +                printf("Strict mode: Image size mismatch!\n");
> > +            }
> > +            goto out;
> > +        } else {
> > +            error_report("Image size mismatch!");
> > +        }
> > +
> > +        if (total_sectors1 > total_sectors2) {
> > +            lo_total_sectors = total_sectors1;
> > +            lo_sector_num = total_sectors2;
> > +            bsover = bs1;
> > +            filename_over = filename1;
> > +        } else {
> > +            lo_total_sectors = total_sectors2;
> > +            lo_sector_num = total_sectors1;
> > +            bsover = bs2;
> > +            filename_over = filename2;
> > +        }
> > +
> > +        progress_base = lo_total_sectors;
> > +
> > +        for (;;) {
> > +            nb_sectors = sectors_to_process(lo_total_sectors,
> > lo_sector_num);
> > +            if (nb_sectors <= 0) {
> > +                break;
> > +            }
> > +            rv = bdrv_is_allocated_above(bsover, NULL,
> > lo_sector_num,
> > +                                         nb_sectors, &pnum);
> > +            nb_sectors = pnum;
> > +            if (rv) {
> > +                rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
> > +                               filename_over, buf1, quiet);
> > +                if (rv) {
> > +                    if (rv < 0) {
> > +                        ret = 2;
> > +                    }
> > +                    goto out;
> > +                }
> > +            }
> > +            lo_sector_num += nb_sectors;
> > +            qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> > +        }
> > +    }
> 
> Hm, nb_sectors is the number of sectors processed in one loop
> iteration,
> isn't it? How can you calculate a meaningful progress from it then?
> And
> even if it did work, wouldn't the progress bar jump backwards when it
> starts comparing the part that exists in both images?
> 

qemu_progress_print is quite a strange function and this is the correct usage even if
it looks suspicious. First argument is delta since last call not the decimal part.

> And didn't we decide that qemu-img compare should always return the
> lowest offset at which the images differ?

No we did not have any final decision yet. There were just two approaches and patch uses the one
I prefer (as the opposite approach was only suggested not requested).

My expectation is that compare subcommand returns (as return value) if the images are same or not. 
Printed info where the images differ is not so important and is suppressed in quiet mode. 
As compare should minimize the time used for compare, it checks non-shared portion of disk first 
as it's faster and probability of difference is bigger.

> 
> > +
> > +
> > +    for (;;) {
> > +        nb_sectors = sectors_to_process(total_sectors,
> > sector_num);
> > +        if (nb_sectors <= 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;
> > +        }
> 
> nb_sectors = MIN(pnum1, pnum2);
> 
> > @@ -114,6 +128,28 @@ it doesn't need to be specified separately in
> > this case.
> >  
> >  Commit the changes recorded in @var{filename} in its base image.
> >  
> > +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q]
> > @var{filename1} @var{filename2}
> > +
> > +Check if two images contains same content. You can compare images
> > with
> 
> s/contains/contain the/
> 
> Or maybe even "have" the same content. Containing content is kind of
> redundant.
> 
> > +different format or settings.
> > +
> > +Format is probed unless you specify it by @var{-f} (used for
> > @var{filename1}) and/or @var{-F} (used for @var{filename2})
> > option.
> 
> s/Format/The format/
> 
> > +
> > +By default, compare evaluates as identical images with different
> > size where
> > +bigger image contains only unallocated and/or zeroed sectors in
> > area above
> > +second image size.
> 
> Hm, let me try and rephrase this one...
> 
> "By default, images with different size are considered identical if
> the
> larger image contains only unallocated and/or zeroed sectors in the
> area
> after the end of the other image."
> 
> > In addition, if any sector is not allocated in one image
> > +and contains only zero bytes in second, it is evaluated as equal.
> 
> s/in second/in the second one/
> 
> > You can use
> > +Strict mode by specifying @var{-s} option. When compare runs in
> > Strict mode,
> 
> _the_ -s option
> 
> > +it fails in case image size differs or sector is allocated in one
> > image and
> 
> _a_ sector
> 
> > +is not allocated in second.
> 
> the second one.
> 
> > +
> > +By default, compare prints out result message. This message
> > displays
> 
> _a_ result message
> 
> > +information that both images are same or position of first
> > different byte.
> 
> _the_ position of _the_ first different byte
> 
> > +In addition, result message can report different image size in
> > case Strict
> > +mode is used.
> 
> _the_ result message can report an image size difference
> 
> > +In case you want to suppress any non-error output, you can use
> > Quiet mode by
> > +specifying @var{-q} option.
> 
> _the_ -q option
> 
> Kevin
>

Patch

diff --git a/block.c b/block.c
index 854ebd6..fdc8c45 100644
--- a/block.c
+++ b/block.c
@@ -2698,6 +2698,7 @@  int bdrv_has_zero_init(BlockDriverState *bs)
 
 typedef struct BdrvCoIsAllocatedData {
     BlockDriverState *bs;
+    BlockDriverState *base;
     int64_t sector_num;
     int nb_sectors;
     int *pnum;
@@ -2828,6 +2829,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;
+}
+
 BlockInfo *bdrv_query_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/block.h b/block.h
index 722c620..2cb8d71 100644
--- a/block.h
+++ b/block.h
@@ -278,6 +278,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, BlockdevOnError on_read_error,
                        BlockdevOnError on_write_error);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index a181363..c076085 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] [-F fmt] [-p] [-q] [-s] filename1 filename2")
+STEXI
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{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 e29e01b..09abb3a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,13 @@  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"
+           "\n"
+           "Parameters to compare subcommand:\n"
+           "  '-f' First image format\n"
+           "  '-F' Second image format\n"
+           "  '-q' Quiet mode - do not print any output (except errors)\n"
+           "  '-s' Strict mode - fail on different image size or sector allocation\n";
 
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);
@@ -658,6 +664,286 @@  static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
 
 #define IO_BUF_SIZE (2 * 1024 * 1024)
 
+static int64_t sectors_to_bytes(int64_t sectors)
+{
+    return sectors << BDRV_SECTOR_BITS;
+}
+
+/*
+ * 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;
+}
+
+/*
+ * Check if passed sectors are filled with 0.
+ *
+ * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
+ * data and negative value on error.
+ */
+static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
+              const char *filename, uint8_t *buffer, int quiet)
+{
+    int pnum, rv = 0;
+    rv = bdrv_read(bs, sect_num, buffer, sect_count);
+    if (rv < 0) {
+        error_report("Error while reading offset %" PRId64 " of %s: %s",
+                     sectors_to_bytes(sect_num), filename, strerror(-rv));
+        return rv;
+    }
+    rv = is_allocated_sectors(buffer, sect_count, &pnum);
+    if (rv || pnum != sect_count) {
+        if (!quiet) {
+            printf("Content mismatch at offset %" PRId64 "!\n",
+                   sectors_to_bytes(rv ? sect_num : sect_num + pnum));
+        }
+        return 1;
+    }
+
+    return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred
+ */
+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 ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+    int progress = 0, quiet = 0, strict = 0;
+    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, "hpf:F:sq");
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case '?':
+        case 'h':
+            help();
+            break;
+        case 'f':
+            fmt1 = optarg;
+            break;
+        case 'F':
+            fmt2 = optarg;
+            break;
+        case 'p':
+            progress = 1;
+            break;
+        case 'q':
+            quiet = 1;
+            break;
+        case 's':
+            strict = 1;
+            break;
+        }
+    }
+
+    /* Progress is not shown in Quiet mode */
+    if (quiet) {
+        progress = 0;
+    }
+
+    if (optind > argc - 2) {
+        help();
+    }
+    filename1 = argv[optind++];
+    filename2 = argv[optind++];
+
+    /* Initialize before goto out */
+    qemu_progress_init(progress, 2.0);
+
+    bs1 = bdrv_new_open(filename1, fmt1, flags, true);
+    if (!bs1) {
+        error_report("Can't open file %s", filename1);
+        ret = 2;
+        goto out3;
+    }
+
+    bs2 = bdrv_new_open(filename2, fmt2, flags, true);
+    if (!bs2) {
+        error_report("Can't open file %s", filename2);
+        ret = 2;
+        goto out2;
+    }
+
+    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 = MIN(total_sectors1, total_sectors2);
+    progress_base = total_sectors;
+
+    qemu_progress_print(0, 100);
+
+    if (total_sectors1 != total_sectors2) {
+        BlockDriverState *bsover;
+        int64_t lo_total_sectors, lo_sector_num;
+        const char *filename_over;
+
+        if (strict) {
+            ret = 1;
+            if (!quiet) {
+                printf("Strict mode: Image size mismatch!\n");
+            }
+            goto out;
+        } else {
+            error_report("Image size mismatch!");
+        }
+
+        if (total_sectors1 > total_sectors2) {
+            lo_total_sectors = total_sectors1;
+            lo_sector_num = total_sectors2;
+            bsover = bs1;
+            filename_over = filename1;
+        } else {
+            lo_total_sectors = total_sectors2;
+            lo_sector_num = total_sectors1;
+            bsover = bs2;
+            filename_over = filename2;
+        }
+
+        progress_base = lo_total_sectors;
+
+        for (;;) {
+            nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
+            if (nb_sectors <= 0) {
+                break;
+            }
+            rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num,
+                                         nb_sectors, &pnum);
+            nb_sectors = pnum;
+            if (rv) {
+                rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
+                               filename_over, buf1, quiet);
+                if (rv) {
+                    if (rv < 0) {
+                        ret = 2;
+                    }
+                    goto out;
+                }
+            }
+            lo_sector_num += nb_sectors;
+            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+        }
+    }
+
+
+    for (;;) {
+        nb_sectors = sectors_to_process(total_sectors, sector_num);
+        if (nb_sectors <= 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 offset %" PRId64 " of %s:"
+                                 " %s", sectors_to_bytes(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 offset %" PRId64
+                                 " of %s: %s", sectors_to_bytes(sector_num),
+                                 filename2, strerror(-rv));
+                    goto out;
+                }
+                rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+                if (rv || pnum != nb_sectors) {
+                    ret = 1;
+                    if (!quiet) {
+                        printf("Content mismatch at offset %" PRId64 "!\n",
+                               sectors_to_bytes(
+                                   rv ? sector_num : sector_num + pnum));
+                    }
+                    goto out;
+                }
+            }
+        } else {
+            if (strict) {
+                ret = 1;
+                if (!quiet) {
+                    printf("Strict mode: Offset %" PRId64
+                           " allocation mismatch!",
+                           sectors_to_bytes(sector_num));
+                }
+                goto out;
+            }
+
+            if (allocated1) {
+                rv = is_zeroed(bs1, sector_num, nb_sectors,
+                               filename1, buf1, quiet);
+            } else {
+                rv = is_zeroed(bs2, sector_num, nb_sectors,
+                               filename2, buf1, quiet);
+            }
+            if (rv) {
+                if (rv < 0) {
+                    ret = 2;
+                }
+                goto out;
+            }
+        }
+        sector_num += nb_sectors;
+        qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+    }
+    if (!quiet) {
+        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;
diff --git a/qemu-img.texi b/qemu-img.texi
index 60b83fc..cb073c3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -81,6 +81,20 @@  deletes a snapshot
 lists all snapshots in the given image
 @end table:
 
+Parameters to compare subcommand:
+
+@table @option
+
+@item -f
+First image format
+@item -F
+Second image format
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
 Command description:
 
 @table @option
@@ -114,6 +128,28 @@  it doesn't need to be specified separately in this case.
 
 Commit the changes recorded in @var{filename} in its base image.
 
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Check if two images contains same content. You can compare images with
+different format or settings.
+
+Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+
+By default, compare evaluates as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+By default, compare prints out result message. This message displays
+information that both images are same or position of first different byte.
+In addition, result message can report different image size in case Strict
+mode is used.
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
 @item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}