Add compare subcommand for qemu-img

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 <[email protected]>

Patch:
--

block.c +39 -0

index b38940b..919f8e9 100644
@@ -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;

block.h +2 -1

index c89590d..6f39da9 100644
@@ -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);

qemu-img-cmds.hx +6 -0

index 39419a0..5b34896 100644
@@ -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

qemu-img.c +47 -1

index 80cfb9b..99a2f16 100644
@@ -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;

qemu-img.c +25 -0

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

qemu-img.c +12 -0

+        }
+
+        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);

qemu-img.c +24 -0

+        }
+    }
+
+    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;

qemu-img.c +14 -0

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

qemu-img.c +7 -0

+    }
+    printf("Images are identical.\n");
+
+out:
+    bdrv_delete(bs2);
+    qemu_vfree(buf1);
+    qemu_vfree(buf2);

From: "Paolo Bonzini" <[email protected]>
To: "Miroslav Rezanina" <[email protected]>
Cc: [email protected]
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:

Quoted 184 lines

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 <[email protected]>

Patch:
--

block.c +19 -0

index b38940b..919f8e9 100644
@@ -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)

block.c +19 -0

+{
+    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;

block.h +2 -1

index c89590d..6f39da9 100644
@@ -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);

qemu-img-cmds.hx +6 -0

index 39419a0..5b34896 100644
@@ -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

qemu-img.c +3 -1

index 80cfb9b..99a2f16 100644
@@ -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);

qemu-img.c +32 -0

@@ -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 */

qemu-img.c +12 -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:g:");
+        if (c == -1) {
+            break;

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 710

Quoted 10 lines

qemu-img.c +10 -0

+        }
+        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.

Quoted 29 lines

qemu-img.c +29 -0

+        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-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 745

Quoted 6 lines

qemu-img.c +6 -0

+    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.

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 749

Quoted 4 lines

qemu-img.c +4 -0

+    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;

Quoted 1 line

+ 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) {
   }

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 761

Quoted 10 lines

qemu-img.c +10 -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.

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 766

Quoted 5 lines

qemu-img.c +5 -0

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

bsover = bs2;

Quoted 1 line

+ filename_over = filename2;

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

Quoted 25 lines

qemu-img.c +6 -0

+        }
+
+        progress_base = over_sectors;
+
+        for (;;) {
+            if ((nb_sectors =

sectors_to_process(over_sectors,over_num)) <= 0)

qemu-img.c +3 -0

+              break;
+
+            rv =

bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum);

qemu-img.c +5 -0

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

qemu-img.c +6 -0

+                   ret = 2;
+                   goto out;
+               }
+               rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+               if (rv || pnum != nb_sectors) {
+                   ret = 1;

Quoted 2 lines

+ 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.

Quoted 6 lines

qemu-img.c +5 -0

+                   goto out;
+               }
+            }
+            over_num += nb_sectors;
+            qemu_progress_print(((float) nb_sectors /

progress_base)*100, 100);

qemu_progress_print(nb_sectors, over_sectors);

Quoted 35 lines

qemu-img.c +5 -0

+        }
+    }
+
+    for (;;) {
+        if ((nb_sectors = sectors_to_process(total_sectors,

sector_num)) <= 0)

qemu-img.c +2 -0

+            break;
+        allocated1 = bdrv_is_allocated_above(bs1, NULL,

sector_num, nb_sectors,

qemu-img.c +2 -0

+                                             &pnum1);
+        allocated2 = bdrv_is_allocated_above(bs2, NULL,

sector_num, nb_sectors,

qemu-img.c +13 -0

+                                             &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));

qemu-img.c +6 -0

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

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 828

Quoted 6 lines

+ " %s", sector_num, filename2,
strerror(-rv));

qemu-img.c +4 -0

+                  goto out;
+              }
+              rv = compare_sectors(buf1,buf2,nb_sectors,&pnum);
+              if (rv || pnum != nb_sectors) {

No need to check pnum != nb_sectors.

Quoted 4 lines

qemu-img.c +2 -0

+                  ret = 1;
+                  printf("Images are different - content

mismatch!\n");

Please print the sector number here.

Quoted 27 lines

qemu-img.c +17 -0

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

qemu-img.c +6 -0

+               goto out;
+           }
+           rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+           if (rv || pnum != nb_sectors) {
+               ret = 1;
+               printf("Images are different - content

Quoted 1 line

mismatch!\n");

Please print the sector number here.

Quoted 6 lines

qemu-img.c +5 -0

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

Quoted 9 lines

qemu-img.c +8 -0

+    }
+    printf("Images are identical.\n");
+
+out:
+    bdrv_delete(bs2);
+    qemu_vfree(buf1);
+    qemu_vfree(buf2);
+out2:

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,

Quoted 10 lines

qemu-img.c +6 -0

+    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

Quoted 5 lines

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?

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 19 lines

Quoted 19 lines

Quoted 4 lines

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 749

Quoted 4 lines

qemu-img.c +4 -0

+    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;

Quoted 1 line

Quoted 1 line

+ 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:

Quoted 3 lines

Quoted 3 lines

   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.

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

qemu-img.c +0 -0

@@ -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)
 

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 <[email protected]>

+++ b/qemu-img.c

Quoted 9 lines

qemu-img.c static void help(void) · line 101

Quoted 9 lines

+++ b/qemu-img.c

qemu-img.c +3 -1

@@ -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.

From: "Eric Blake" <[email protected]>
To: "Miroslav Rezanina" <[email protected]>
Cc: [email protected], "Paolo Bonzini" <[email protected]>
Sent: Wednesday, August 1, 2012 3:21:03 PM
Subject: Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img

Quoted 16 lines · Miroslav Rezanina

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 <[email protected]>

+++ b/qemu-img.c

qemu-img.c static void help(void) · line 103

Quoted 9 lines

qemu-img.c +3 -1

@@ -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.

--
Eric Blake [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org

From: "Paolo Bonzini" <[email protected]>
To: "Miroslav Rezanina" <[email protected]>
Cc: [email protected]
Sent: Wednesday, August 1, 2012 12:22:50 PM
Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img

Quoted 6 lines

qemu-img.c +5 -0

+                   goto out;
+               }
+            }
+            over_num += nb_sectors;
+            qemu_progress_print(((float) nb_sectors /

progress_base)*100, 100);

qemu_progress_print(nb_sectors, over_sectors);

Paolo

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

+ qemu_progress_print(((float) nb_sectors /
progress_base)*100, 100);

qemu_progress_print(nb_sectors, over_sectors);

Quoted 2 lines

Quoted 2 lines

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

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.

qemu-img-cmds.hx +4 -0

+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

qemu-img.c +0 -0

index 80cfb9b..6722fa0 100644

qemu-img.c +1 -0

@@ -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}

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.

qemu-img.c +25 -0

+            }
+            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) {

qemu-img.c +38 -0

+                    }
+                    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

From: "Kevin Wolf" <[email protected]>
To: "Miroslav Rezanina" <[email protected]>
Cc: [email protected], [email protected]
Sent: Tuesday, November 20, 2012 1:36:37 PM
Subject: Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img

Am 03.08.2012 08:45, schrieb Miroslav Rezanina:

Quoted 53 lines

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).

Quoted 5 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 776

Quoted 12 lines

qemu-img.c +11 -0

+    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?

Quoted 39 lines

qemu-img.c +24 -0

+            }
+            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);

qemu-img.c +4 -0

+            if (nb_sectors <= 0) {
+                break;
+            }
+            rv = bdrv_is_allocated(bsover, lo_sector_num,

nb_sectors, &pnum);

qemu-img.c +3 -0

+            nb_sectors = pnum;
+            if (rv) {
+                rv = bdrv_read(bsover, lo_sector_num, buf1,

nb_sectors);

(quoted diff) +2 -0

+                if (rv < 0) {
+                    error_report("error while reading sector %"

PRId64
+ " of %s: %s", lo_sector_num,

Quoted 2 lines

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.

Quoted 11 lines

qemu-img.c +4 -0

+                    ret = 2;
+                    goto out;
+                }
+                rv = is_allocated_sectors(buf1, nb_sectors,

&pnum);

qemu-img.c +4 -0

+                if (rv || pnum != nb_sectors) {
+                    ret = 1;
+                    if (!quiet) {
+                        printf("Content mismatch - Sector %"

PRId64

Review

Quoted 3 lines

(quoted diff) +3 -0

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

Quoted 54 lines

qemu-img.c +6 -0

+                    }
+                    goto out;
+                }
+            }
+            lo_sector_num += nb_sectors;
+            qemu_progress_print(((float) nb_sectors /

progress_base)*100, 100);

qemu-img.c +6 -0

+        }
+    }
+
+
+    for (;;) {
+        nb_sectors = sectors_to_process(total_sectors,

sector_num);

qemu-img.c +4 -0

+        if (nb_sectors <= 0) {
+            break;
+        }
+        allocated1 = bdrv_is_allocated_above(bs1, NULL,

sector_num, nb_sectors,

(quoted diff) +2 -0

+                                             &pnum1);
+        allocated2 = bdrv_is_allocated_above(bs2, NULL,

sector_num, nb_sectors,

qemu-img.c +13 -0

+                                             &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));

qemu-img.c +6 -0

+                    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,

qemu-img.c +4 -0

+                                 strerror(-rv));
+                    goto out;
+                }
+                rv = compare_sectors(buf1, buf2, nb_sectors,

&pnum);
+ if (rv || pnum != nb_sectors) {

Quoted 6 lines

qemu-img.c +3 -0

+                    ret = 1;
+                    if (!quiet) {
+                        printf("Content mismatch at sector %"

PRId64 "!\n",
+ rv ? sector_num : sector_num +
pnum);

Same here.

Kevin

qemu-img.c +11 -0

+    for (;;) {
+        c = getopt(argc, argv, "pf:F:sq");
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case 'f':
+            fmt1 = optarg;
+            break;
+        case 'F':
+            fmt2 = optarg;

qemu-img.c +9 -0

+        help();
+        goto out3;
+    }
+    filename1 = argv[optind++];
+    filename2 = argv[optind++];
+
+    /* Initialize before goto out */
+    qemu_progress_init(progress, 2.0);
+

qemu-img.c +42 -0

+        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!\n");
+            }
+            goto out;
+        } else {
+            error_report("Image size mismatch!");
+        }
+
+        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;

qemu-img.c +11 -0

+            nb_sectors = pnum;
+            if (rv) {
+                rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
+                if (rv < 0) {
+                    error_report("error while reading data offset %" PRId64
+                                 " of %s: %s", sector_to_bytes(lo_sector_num),
+                                 filename_over, strerror(-rv));
+                    ret = 2;
+                    goto out;
+                }
+                rv = is_allocated_sectors(buf1, nb_sectors, &pnum);

qemu-img.c +7 -1

index e29e01b..9cc4365 100644
@@ -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);

qemu-img.c +48 -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, "pf:F:sq");
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        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;
+        }

qemu-img.c +18 -0

+    }
+    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;

qemu-img.c +46 -0

+    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) {
+            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_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);

qemu-img.c +13 -0

+    for (;;) {
+        c = getopt(argc, argv, "pf:F:sq");
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case 'f':
+            fmt1 = optarg;
+            break;
+        case 'F':
+            fmt2 = optarg;
+            break;
+        case 'p':

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 <[email protected]>

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 <[email protected]>

block.c +39 -0

index 854ebd6..fdc8c45 100644
@@ -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));

block.h +0 -0

index 722c620..2cb8d71 100644

qemu-img-cmds.hx +6 -0

index a181363..c076085 100644
@@ -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

qemu-img.c +0 -0

index e29e01b..09abb3a 100644
@@ -101,7 +101,13 @@ static void help(void)
            "  '-a' applies a snapshot (revert disk to saved state)\n"

qemu-img.c +0 -0

     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)

qemu-img.c +13 -0

+        }
+        return 1;
+    }
+
+    return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred

qemu-img.c +116 -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) {

qemu-img.c +6 -0

+
+
+    for (;;) {
+        nb_sectors = sectors_to_process(total_sectors, sector_num);
+        if (nb_sectors <= 0) {
+            break;

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.

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

From: "Kevin Wolf" <[email protected]>
To: "Miroslav Rezanina" <[email protected]>
Cc: [email protected], "Paolo Bonzini" <[email protected]>, "Stefan Hajnoczi" <[email protected]>
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:

Quoted 41 lines

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

Quoted 1 line

Signed-off-by Miroslav Rezanina <[email protected]>

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

Quoted 78 lines

block.c +10 -0

index 854ebd6..fdc8c45 100644
@@ -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);

block.c +9 -0

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

block.c +19 -0

+{
+    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));

block.h +2 -0

index 722c620..2cb8d71 100644
@@ -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);

Quoted 3 lines

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.

Quoted 36 lines

qemu-img-cmds.hx +6 -0

index a181363..c076085 100644
@@ -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

qemu-img.c +5 -1

index e29e01b..09abb3a 100644
@@ -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"

Quoted 4 lines

qemu-img.c +1 -0

+           "  '-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.

Quoted 8 lines

 
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);

qemu-img.c +0 -0

@@ -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)
 

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 675

Quoted 9 lines

qemu-img.c +9 -0

+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.

Quoted 4 lines

qemu-img.c +4 -0

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

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 684

Quoted 5 lines

qemu-img.c +5 -0

+        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'.

Quoted 11 lines

qemu-img.c +5 -0

+
+/*
+ * 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".

Quoted 8 lines

qemu-img.c +5 -0

+{
+    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.

Quoted 8 lines

qemu-img.c +7 -0

+        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.

Quoted 13 lines

qemu-img.c +13 -0

+        }
+        return 1;
+    }
+
+    return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred

qemu-img.c static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, · line 729

Quoted 10 lines

qemu-img.c +10 -0

+ */
+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.

Quoted 3 lines

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

bool

Quoted 128 lines

qemu-img.c +106 -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);

qemu-img.c +4 -0

+            if (nb_sectors <= 0) {
+                break;
+            }
+            rv = bdrv_is_allocated_above(bsover, NULL,

lo_sector_num,

qemu-img.c +14 -0

+                                         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);
+ }