Re: [Qemu-devel] [PATCH v3] Add compare subcommand for qemu-img

2012-11-22 Thread Stefan Hajnoczi
On Wed, Nov 21, 2012 at 04:50:14AM -0500, Miroslav Rezanina wrote:
 diff --git a/qemu-img.c b/qemu-img.c
 index e29e01b..6294b11 100644
 --- a/qemu-img.c
 +++ b/qemu-img.c
 @@ -101,7 +101,12 @@ 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

Please add \n to separate like the other parameter docs for
subcommands.

 +   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);
 @@ -657,6 +662,277 @@ static int compare_sectors(const uint8_t *buf1, const 
 uint8_t *buf2, int n,
  }
  
  #define IO_BUF_SIZE (2 * 1024 * 1024)
 +#define sector_to_bytes(sector) ((sector)  BDRV_SECTOR_BITS)

No macros, please.  Hiding the sector conversion isn't consistent anyway
since further down in your patch you explicitly use  BDRV_SECTOR_BITS.

Either open code or define static inline functions so we have type
information.

 +
 +/*
 + * Get number of sectors that can be stored in IO buffer.
 + */
 +
 +static int64_t sectors_to_process(int64_t total, int64_t from)

Doc comments fit snuggly against the function definition, no newline
please.  QEMU code isn't very consistent in doc comment formatting in
general but it does not use a newline here.

 +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 = (quiet == 0) ? 1 : 0;
 +break;
 +case 'q':
 +quiet = 1;
 +if (progress == 1) {
 +progress = 0;
 +}
 +break;

It's cleaner to silence progress after all options have been parsed than
to duplicate the quiet/progress checking.

/* -q overrides -p */
if (quiet) {
progress = 0;
}

 +case 's':
 +strict = 1;
 +break;
 +}

case 'h':
case '?':
help();
break;

 +}
 +if (optind = argc) {

This subcommand takes two filenames, so check the number of arguments is
correct:

if (optind + 1 = 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, 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);

Stray ':'?

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

We should error out if a backing image has non-zero data:

rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num, nb_sectors 

[Qemu-devel] [PATCH v3] Add compare subcommand for qemu-img

2012-11-21 Thread 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

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


Signed-off-by: Miroslav Rezanina

diff --git a/block.c b/block.c
index 854ebd6..fdc8c45 100644
--- a/block.c
+++ b/block.c
@@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
 
 typedef struct BdrvCoIsAllocatedData {
 BlockDriverState *bs;
+BlockDriverState *base;
 int64_t sector_num;
 int nb_sectors;
 int *pnum;
@@ -2828,6 +2829,44 @@ int coroutine_fn 
bdrv_co_is_allocated_above(BlockDriverState *top,
 return 0;
 }
 
+/* Coroutine wrapper for bdrv_is_allocated_above() */
+static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
+{
+BdrvCoIsAllocatedData *data = opaque;
+BlockDriverState *top = data-bs;
+BlockDriverState *base = data-base;
+
+data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num,
+   data-nb_sectors, data-pnum);
+data-done = true;
+}
+
+/*
+ * Synchronous wrapper around bdrv_co_is_allocated_above().
+ *
+ * See bdrv_co_is_allocated_above() for details.
+ */
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+  int64_t sector_num, int nb_sectors, int *pnum)
+{
+Coroutine *co;
+BdrvCoIsAllocatedData data = {
+.bs = top,
+.base = base,
+.sector_num = sector_num,
+.nb_sectors = nb_sectors,
+.pnum = pnum,
+.done = false,
+};
+
+co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
+qemu_coroutine_enter(co, data);
+while (!data.done) {
+qemu_aio_wait();
+}
+return data.ret;
+}
+
 BlockInfo *bdrv_query_info(BlockDriverState *bs)
 {
 BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/block.h b/block.h
index 722c620..2cb8d71 100644
--- a/block.h
+++ b/block.h
@@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t 
sector_num, int nb_sectors);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
   int *pnum);
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+int64_t sector_num, int nb_sectors, int *pnum);
 
 void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
BlockdevOnError on_write_error);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index a181363..c076085 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -27,6 +27,12 @@ STEXI
 @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
 ETEXI
 
+DEF(compare, img_compare,
+compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2)
+STEXI
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} 
@var{filename2}
+ETEXI
+
 DEF(convert, img_convert,
 convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s 
snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename)
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index e29e01b..6294b11 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,12 @@ 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' 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);
@@ -657,6 +662,277 @@ static int compare_sectors(const uint8_t *buf1, const 
uint8_t *buf2, int n,
 }
 
 #define IO_BUF_SIZE (2 * 1024 * 1024)
+#define sector_to_bytes(sector) ((sector)  BDRV_SECTOR_BITS)
+
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+int64_t rv = total - from;
+
+if (rv  (IO_BUF_SIZE  BDRV_SECTOR_BITS)) {
+return IO_BUF_SIZE  BDRV_SECTOR_BITS;
+}
+
+