Patchwork [v2,RFC] Add compare subcommand for qemu-img

login
register
mail settings
Submitter Miroslav Rezanina
Date Aug. 3, 2012, 6:45 a.m.
Message ID <20120803064520.GA10029@lws.brq.redhat.com>
Download mbox | patch
Permalink /patch/174896/
State New
Headers show

Comments

Miroslav Rezanina - Aug. 3, 2012, 6:45 a.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

v2:
 - changed option for second image format to -F
 - changed handlig 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

[1] Original patch handling was as following:
 i)   neither -f nor -F  - both images probed for type
 ii)  -f only            - both images use specified type
 iii) -F only            - first image probed, second image use specified type
 iii) -f and -F          - first image use -f type, second use -F type

This patch change behavior in way that case ii) and iii) has same efect - we
use specified value for both images.

[2] When we hit different sector we print its number out.

Points to dicuss:

i) Handling -f/-F options.
Currently we have three scenarios - no option
specified - probe all, one of options specified - use it for both, both option
specified - use each value for related image. This behavior is based on idea
that we can use format probing for all images or specify format for all images.
This preserve state when -f fmt specify input image format (compare is only
subcomand with more than one input image except convert that uses multiple
images without possibility to specify different format for each image).

However, there's one more behavior to be considered - to use -f/-F for one
image only - when only one option is provided, only appropriate image use specified
format, second one is probed.

ii) How to handle images with different size.
If size of images is different and strict mode is not used, addditional size of
bigger image is checked to be zeroed/unallocated. This version do this check
before rest of image is compared. This is done to not compare whole image in
case that one of images is only expanded copy of other.

Paolo Bonzini proposed to do this check after compare shared size of images to
go through image sequentially.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 block.c          |   39 ++++++++
 block.h          |    3 +-
 qemu-img-cmds.hx |    6 +
 qemu-img.c       |  277 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-img.texi    |   33 +++++++
 5 files changed, 356 insertions(+), 2 deletions(-)

--
Eric Blake - Aug. 3, 2012, 3:23 p.m.
On 08/03/2012 12:45 AM, 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
> 
> v2:
>  - changed option for second image format to -F
>  - changed handlig 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
> 
> [1] Original patch handling was as following:
>  i)   neither -f nor -F  - both images probed for type
>  ii)  -f only            - both images use specified type
>  iii) -F only            - first image probed, second image use specified type
>  iii) -f and -F          - first image use -f type, second use -F type
> 
> This patch change behavior in way that case ii) and iii) has same efect - we
> use specified value for both images.

I still think orthogonality is better than applying one option to both
files.  Probing is sometimes useful, and you have left no way to probe
one file but not the other.

> 
> [2] When we hit different sector we print its number out.
> 
> Points to dicuss:
> 
> i) Handling -f/-F options.
> Currently we have three scenarios - no option
> specified - probe all, one of options specified - use it for both, both option
> specified - use each value for related image. This behavior is based on idea
> that we can use format probing for all images or specify format for all images.
> This preserve state when -f fmt specify input image format (compare is only
> subcomand with more than one input image except convert that uses multiple
> images without possibility to specify different format for each image).
> 
> However, there's one more behavior to be considered - to use -f/-F for one
> image only - when only one option is provided, only appropriate image use specified
> format, second one is probed.

I would prefer this, as it would let me compare against a file of
unknown type.


> +++ 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")

Out of date with the rest of your patch.

> +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 80cfb9b..6722fa0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -96,7 +96,11 @@ 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"
> +           "  '-F' Second image format (in case it differs from first image)\n"

If you make -f and -F orthogonal, applying to one image each, this might
be better worded as:

'-F' Second image format (-f applies only to first image)\n

or even just

'-F' Second image format


> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occured

s/occured/occurred/


> +++ b/qemu-img.texi
> @@ -67,6 +67,18 @@ deletes a snapshot
>  lists all snapshots in the given image
>  @end table
>  
> +Parameters to compare subcommand:
> +
> +@table @option
> +
> +@item -F
> +Second image format (in case it differs from first image)

Another instance of wording to be careful of.

> @@ -100,6 +112,27 @@ 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}
> +
> +Compare content of two images. You can compare images with different format or
> +settings.
> +
> +Format is probed unless you specify it by @var{-f} and/or @var{-F} option.
> +If only one of these options is specified, it is used for both images.
> +If both options are specfied, @var{-f} is used for @var{filename1} and
> +@var{-F} for @var{filename2}.
> +
> +By default, compare evaluate as identical images with different size where

s/evaluate/evaluates/

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

When -q is not in use, what gets output?  Is it like cmp(1), where
output is silent on the same, and lists the location of the first
differing byte on different?
Kevin Wolf - Nov. 20, 2012, 12:36 p.m.
Am 03.08.2012 08:45, 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
> 
> v2:
>  - changed option for second image format to -F
>  - changed handlig 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
> 
> [1] Original patch handling was as following:
>  i)   neither -f nor -F  - both images probed for type
>  ii)  -f only            - both images use specified type
>  iii) -F only            - first image probed, second image use specified type
>  iii) -f and -F          - first image use -f type, second use -F type
> 
> This patch change behavior in way that case ii) and iii) has same efect - we
> use specified value for both images.
> 
> [2] When we hit different sector we print its number out.
> 
> Points to dicuss:
> 
> i) Handling -f/-F options.
> Currently we have three scenarios - no option
> specified - probe all, one of options specified - use it for both, both option
> specified - use each value for related image. This behavior is based on idea
> that we can use format probing for all images or specify format for all images.
> This preserve state when -f fmt specify input image format (compare is only
> subcomand with more than one input image except convert that uses multiple
> images without possibility to specify different format for each image).
> 
> However, there's one more behavior to be considered - to use -f/-F for one
> image only - when only one option is provided, only appropriate image use specified
> format, second one is probed.

Like Eric, I would prefer this alternative behaviour, so that one option
is clearly related to only one image.

> ii) How to handle images with different size.
> If size of images is different and strict mode is not used, addditional size of
> bigger image is checked to be zeroed/unallocated. This version do this check
> before rest of image is compared. This is done to not compare whole image in
> case that one of images is only expanded copy of other.
> 
> Paolo Bonzini proposed to do this check after compare shared size of images to
> go through image sequentially.

I think the expected semantics is that the tool doesn't print the offset
of just any difference, but of the first one. So I'd agree with Paolo here.

> +    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!");

Missing \n?

I also wonder if it would make sense to implement something like a
qprintf(bool quiet, const char* fmt, ...) so that you don't have this
verbose if (!quiet) around each message.

Also, shouldn't this one be on stderr and be displayed even with -q?

> +            }
> +            goto out;
> +        } else if (!quiet) {
> +            printf("Warning: Image size mismatch!\n");
> +        }
> +
> +        if (total_sectors1 > total_sectors2) {
> +            total_sectors = total_sectors2;
> +            lo_total_sectors = total_sectors1;
> +            lo_sector_num = total_sectors2;
> +            bsover = bs1;
> +            filename_over = filename1;
> +        } else {
> +            total_sectors = total_sectors1;
> +            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(bsover, lo_sector_num, nb_sectors, &pnum);
> +            nb_sectors = pnum;
> +            if (rv) {
> +                rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
> +                if (rv < 0) {
> +                    error_report("error while reading sector %" PRId64
> +                                 " of %s: %s", lo_sector_num, filename_over,
> +                                 strerror(-rv));

Please change the unit of all offsets from sectors to bytes, it's much
friendlier if you don't have to know that qemu uses an arbitrary unit of
512 bytes internally.

> +                    ret = 2;
> +                    goto out;
> +                }
> +                rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> +                if (rv || pnum != nb_sectors) {
> +                    ret = 1;
> +                    if (!quiet) {
> +                        printf("Content mismatch - Sector %" PRId64
> +                               " not available in both images!\n",
> +                               rv ? lo_sector_num : lo_sector_num + pnum);

Same here, plus stderr and display even with -q? (More instances follow,
won't comment on each)

> +                    }
> +                    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 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;
> +                    if (!quiet) {
> +                        printf("Content mismatch at sector %" PRId64 "!\n",
> +                               rv ? sector_num : sector_num + pnum);

Same here.

Kevin

Patch

diff --git a/block.c b/block.c
index b38940b..3558bf9 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..e520eec 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..7ee0f69 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 @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 80cfb9b..6722fa0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -96,7 +96,11 @@  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"
+           "  '-F' Second image format (in case it differs from first image)\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);
@@ -652,6 +656,277 @@  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 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, "pf:F:sq");
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case 'f':
+            fmt1 = optarg;
+            if (fmt2 == NULL) {
+                fmt2 = optarg;
+            }
+            break;
+        case 'F':
+            fmt2 = optarg;
+            if (fmt1 == NULL) {
+                fmt2 = optarg;
+            }
+            break;
+        case 'p':
+            progress = (quiet == 0) ? 1 : 0;
+            break;
+        case 'q':
+            quiet = 1;
+            if (progress == 1) {
+                progress = 0;
+            }
+            break;
+        case 's':
+            strict = 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;
+    }
+
+    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;
+
+    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!");
+            }
+            goto out;
+        } else if (!quiet) {
+            printf("Warning: Image size mismatch!\n");
+        }
+
+        if (total_sectors1 > total_sectors2) {
+            total_sectors = total_sectors2;
+            lo_total_sectors = total_sectors1;
+            lo_sector_num = total_sectors2;
+            bsover = bs1;
+            filename_over = filename1;
+        } else {
+            total_sectors = total_sectors1;
+            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(bsover, lo_sector_num, nb_sectors, &pnum);
+            nb_sectors = pnum;
+            if (rv) {
+                rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
+                if (rv < 0) {
+                    error_report("error while reading sector %" PRId64
+                                 " of %s: %s", lo_sector_num, filename_over,
+                                 strerror(-rv));
+                    ret = 2;
+                    goto out;
+                }
+                rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+                if (rv || pnum != nb_sectors) {
+                    ret = 1;
+                    if (!quiet) {
+                        printf("Content mismatch - Sector %" PRId64
+                               " not available in both images!\n",
+                               rv ? lo_sector_num : lo_sector_num + pnum);
+                    }
+                    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 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;
+                    if (!quiet) {
+                        printf("Content mismatch at sector %" PRId64 "!\n",
+                               rv ? sector_num : sector_num + pnum);
+                    }
+                    goto out;
+                }
+            }
+        } else {
+            BlockDriverState *bstmp;
+            const char *filenametmp;
+
+            if (strict) {
+                ret = 1;
+                if (!quiet) {
+                    printf("Strict mode: Sector %" PRId64
+                           " allocation mismatch!",
+                           sector_num);
+                }
+                goto out;
+            }
+
+            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;
+                if (!quiet) {
+                    printf("Content mismatch at sector %" PRId64 "!\n",
+                           rv ? sector_num : sector_num + pnum);
+                }
+                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 77c6d0b..2972118 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -67,6 +67,18 @@  deletes a snapshot
 lists all snapshots in the given image
 @end table
 
+Parameters to compare subcommand:
+
+@table @option
+
+@item -F
+Second image format (in case it differs from first image)
+@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
@@ -100,6 +112,27 @@  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}
+
+Compare content of two images. You can compare images with different format or
+settings.
+
+Format is probed unless you specify it by @var{-f} and/or @var{-F} option.
+If only one of these options is specified, it is used for both images.
+If both options are specfied, @var{-f} is used for @var{filename1} and
+@var{-F} for @var{filename2}.
+
+By default, compare evaluate 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.
+
+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}