| Message ID | 490157254.6767622.1343815423950.JavaMail.root@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 01/08/2012 12:03, Miroslav Rezanina ha scritto: > This patch adds compare subcommand that compares two images. Compare has following criteria: > - only data part is compared > - unallocated sectors are not read > - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest > - qemu-img returns: > - 0 if images are identical > - 1 if images differ > - 2 on error > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > Patch: > -- > diff --git a/block.c b/block.c > index b38940b..919f8e9 100644 > --- a/block.c > +++ b/block.c > @@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) > > typedef struct BdrvCoIsAllocatedData { > BlockDriverState *bs; > + BlockDriverState *base; > int64_t sector_num; > int nb_sectors; > int *pnum; > @@ -2414,6 +2415,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, > return 0; > } > > +/* Coroutine wrapper for bdrv_is_allocated_above() */ > +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) > +{ > + BdrvCoIsAllocatedData *data = opaque; > + BlockDriverState *top = data->bs; > + BlockDriverState *base = data->base; > + > + data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num, > + data->nb_sectors,data->pnum); > + data->done = true; > +} > + > +/* > + * Synchronous wrapper around bdrv_co_is_allocated_above(). > + * > + * See bdrv_co_is_allocated_above() for details. > + */ > +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, > + int64_t sector_num, int nb_sectors, int *pnum) > +{ > + Coroutine *co; > + BdrvCoIsAllocatedData data = { > + .bs = top, > + .base = base, > + .sector_num = sector_num, > + .nb_sectors = nb_sectors, > + .pnum = pnum, > + .done = false, > + }; > + > + co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); > + qemu_coroutine_enter(co, &data); > + while (!data.done) { > + qemu_aio_wait(); > + } > + return data.ret; > +} > + > BlockInfoList *qmp_query_block(Error **errp) > { > BlockInfoList *head = NULL, *cur_item = NULL; > diff --git a/block.h b/block.h > index c89590d..6f39da9 100644 > --- a/block.h > +++ b/block.h > @@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); > int bdrv_has_zero_init(BlockDriverState *bs); > int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > int *pnum); > - > +int bdrv_is_allocated_above(BlockDriverState* top, BlockDriverState *base, > + int64_t sector_num, int nb_sectors, int *pnum); > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > BlockErrorAction on_write_error); > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > index 39419a0..5b34896 100644 > --- a/qemu-img-cmds.hx > +++ b/qemu-img-cmds.hx > @@ -27,6 +27,12 @@ STEXI > @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} > ETEXI > > +DEF("compare", img_compare, > + "compare [-f fmt] [-g fmt] [-p] filename1 filename2") > +STEXI > +@item compare [-f fmt] [-g fmt] [-p] filename1 filename2 > +ETEXI > + > DEF("convert", img_convert, > "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename") > STEXI > diff --git a/qemu-img.c b/qemu-img.c > index 80cfb9b..99a2f16 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -96,7 +96,9 @@ static void help(void) > " '-a' applies a snapshot (revert disk to saved state)\n" > " '-c' creates a snapshot\n" > " '-d' deletes a snapshot\n" > - " '-l' lists all snapshots in the given image\n"; > + " '-l' lists all snapshots in the given image\n" > + "Parameters to compare subcommand:\n" > + " '-g' Second image format (in case it differs from first image)\n"; > > printf("%s\nSupported formats:", help_msg); > bdrv_iterate_format(format_print, NULL); > @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, > > #define IO_BUF_SIZE (2 * 1024 * 1024) > > +/* > + * Get number of sectors that can be stored in IO buffer. > + */ > + > +static int64_t sectors_to_process(int64_t total, int64_t from) > +{ > + int64_t rv = total - from; > + > + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) > + return IO_BUF_SIZE >> BDRV_SECTOR_BITS; > + > + return rv; > +} > + > +/* > + * Compares two images. Exit codes: > + * > + * 0 - Images are identical > + * 1 - Images differ > + * 2 - Error occured > + */ > + > +static int img_compare(int argc, char **argv) > +{ > + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; > + BlockDriverState *bs1, *bs2; > + int64_t total_sectors1, total_sectors2; > + uint8_t *buf1 = NULL, *buf2 = NULL; > + int pnum1, pnum2; > + int allocated1,allocated2; > + int flags = BDRV_O_FLAGS; > + int progress=0,ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */ > + int64_t total_sectors; > + int64_t sector_num = 0; > + int64_t nb_sectors; > + int c, rv, pnum; > + uint64_t bs_sectors; > + uint64_t progress_base; > + > + > + for(;;) { > + c = getopt(argc, argv, "pf:g:"); > + if (c == -1) { > + break; > + } > + switch(c) { > + case 'f': > + fmt1 = optarg; > + if (fmt2 == NULL) > + fmt2 = optarg; > + break; > + case 'g': > + fmt2 = optarg; > + break; I can guess why you chose 'g', but perhaps 'F' for consistency with qemu-img rebase? Also, perhaps we can add a "strict" mode (-S) that would fail if the images are of different size, and if a sector is allocated in one but unallocated in the other? And a "silent" or "quiet" mode (-s or -q, your choice) that prints nothing, too. > + case 'p': > + progress = 1; > + break; > + } > + } > + if (optind >= argc) { > + help(); > + goto out3; > + } > + filename1 = argv[optind++]; > + filename2 = argv[optind++]; > + > + /* Initialize before goto out */ > + qemu_progress_init(progress, 2.0); > + > + bs1 = bdrv_new_open(filename1, fmt1, flags); > + if (!bs1) { > + error_report("Can't open file %s", filename1); > + ret = 2; > + goto out3; > + } > + > + bs2 = bdrv_new_open(filename2, fmt2, flags); > + if (!bs2) { > + error_report("Can't open file %s:",filename2); > + ret = 2; > + goto out2; > + } > + > + qemu_progress_print(0, 100); > + > + buf1 = qemu_blockalign(bs1,IO_BUF_SIZE); > + buf2 = qemu_blockalign(bs2,IO_BUF_SIZE); > + bdrv_get_geometry(bs1,&bs_sectors); > + total_sectors1 = bs_sectors; You can make total_sectors1/2 unsigned, and avoid the assignment. > + bdrv_get_geometry(bs2,&bs_sectors); > + total_sectors2 = bs_sectors; > + total_sectors = total_sectors1; > + progress_base = total_sectors; over_sectors = MAX(total_sectors1, total_sectors2); total_sector = MIN(total_sectors1, total_sectors2); over_num = over_sectors - total_sectors; > + if (total_sectors1 != total_sectors2) { Using MAX/MIN as above, you can move this part to after you checked the common part of the image, so that images are read sequentially. You can still place the test here in strict mode. With the assignments given above, you can do the test simply like this: if (over_num > 0) { } > + BlockDriverState *bsover; > + int64_t over_sectors, over_num; > + const char *filename_over; > + > + if (total_sectors1 > total_sectors2) { > + total_sectors = total_sectors2; > + over_sectors = total_sectors1; > + over_num = total_sectors2; > + bsover = bs1; > + filename_over = filename1; Only bsover/filename_over are needed of course. > + } else { > + total_sectors = total_sectors1; > + over_sectors = total_sectors2; > + over_num = total_sectors1; > + bsover = bs1; bsover = bs2; > + filename_over = filename2; Again, of course only bsover/filename_over are needed with the changes suggested above. > + } > + > + progress_base = over_sectors; > + > + for (;;) { > + if ((nb_sectors = sectors_to_process(over_sectors,over_num)) <= 0) > + break; > + > + rv = bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum); > + nb_sectors = pnum; > + if (rv) { > + rv = bdrv_read(bsover, over_num, buf1, nb_sectors); > + if (rv < 0) { > + error_report("error while reading sector %" PRId64 " of %s:" > + " %s",over_num, filename_over, strerror(-rv)); > + ret = 2; > + goto out; > + } > + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); > + if (rv || pnum != nb_sectors) { > + ret = 1; > + printf("Images are different - image size mismatch!\n"); The error is not too precise. Let's print "content mismatch at sector 123456", and warn if the images are of different size, but we are still exiting with code 0. > + goto out; > + } > + } > + over_num += nb_sectors; > + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100); qemu_progress_print(nb_sectors, over_sectors); > + } > + } > + > + for (;;) { > + if ((nb_sectors = sectors_to_process(total_sectors, sector_num)) <= 0) > + break; > + allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors, > + &pnum1); > + allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors, > + &pnum2); > + if (pnum1 < pnum2) { > + nb_sectors = pnum1; > + } else { > + nb_sectors = pnum2; > + } > + > + if (allocated1 == allocated2) { > + if (allocated1) { > + rv = bdrv_read(bs1, sector_num, buf1, nb_sectors); > + if (rv < 0) { > + ret = 2; > + error_report("error while reading sector %" PRId64 " of %s:" > + " %s", sector_num, filename1, strerror(-rv)); > + goto out; > + } > + rv = bdrv_read(bs2,sector_num,buf2, nb_sectors); > + if (rv < 0) { > + ret = 2; > + error_report("error while reading sector %" PRId64 " of %s:" > + " %s", sector_num, filename2, strerror(-rv)); > + goto out; > + } > + rv = compare_sectors(buf1,buf2,nb_sectors,&pnum); > + if (rv || pnum != nb_sectors) { No need to check pnum != nb_sectors. > + ret = 1; > + printf("Images are different - content mismatch!\n"); Please print the sector number here. > + goto out; > + } > + } > + } else { > + BlockDriverState *bstmp; > + const char* filenametmp; > + if (allocated1) { > + bstmp = bs1; > + filenametmp = filename1; > + } else { > + bstmp = bs2; > + filenametmp = filename2; > + } > + rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors); > + if (rv < 0) { > + ret = 2; > + error_report("error while reading sector %" PRId64 " of %s: %s", > + sector_num, filenametmp, strerror(-rv)); > + goto out; > + } > + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); > + if (rv || pnum != nb_sectors) { > + ret = 1; > + printf("Images are different - content mismatch!\n"); Please print the sector number here. > + goto out; > + } > + } > + sector_num += nb_sectors; > + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100); qemu_progress_print(nb_sectors, over_sectors); Perhaps larger_sectors is a better name than over_sectors (and larger_only_sectors instead of over_num? but I like this one a bit less...). > + } > + printf("Images are identical.\n"); > + > +out: > + bdrv_delete(bs2); > + qemu_vfree(buf1); > + qemu_vfree(buf2); > +out2: > + bdrv_delete(bs1); > +out3: > + qemu_progress_end(); > + return ret; > +} > + > static int img_convert(int argc, char **argv) > { > int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors; > Only cosmetic problems overall; looks pretty good! Paolo
----- Original Message ----- > From: "Paolo Bonzini" <pbonzini@redhat.com> > To: "Miroslav Rezanina" <mrezanin@redhat.com> > Cc: qemu-devel@nongnu.org > Sent: Wednesday, August 1, 2012 12:22:50 PM > Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img > > Il 01/08/2012 12:03, Miroslav Rezanina ha scritto: > > This patch adds compare subcommand that compares two images. > > Compare has following criteria: > > - only data part is compared > > - unallocated sectors are not read > > - in case of different image size, exceeding part of bigger disk > > has to be zeroed/unallocated to compare rest > > - qemu-img returns: > > - 0 if images are identical > > - 1 if images differ > > - 2 on error > > > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > > > Patch: > > -- > > diff --git a/block.c b/block.c > > index b38940b..919f8e9 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) > > > > typedef struct BdrvCoIsAllocatedData { > > BlockDriverState *bs; > > + BlockDriverState *base; > > int64_t sector_num; > > int nb_sectors; > > int *pnum; > > @@ -2414,6 +2415,44 @@ int coroutine_fn > > bdrv_co_is_allocated_above(BlockDriverState *top, > > return 0; > > } > > > > +/* Coroutine wrapper for bdrv_is_allocated_above() */ > > +static void coroutine_fn bdrv_is_allocated_above_co_entry(void > > *opaque) > > +{ > > + BdrvCoIsAllocatedData *data = opaque; > > + BlockDriverState *top = data->bs; > > + BlockDriverState *base = data->base; > > + > > + data->ret = bdrv_co_is_allocated_above(top, base, > > data->sector_num, > > + > > data->nb_sectors,data->pnum); > > + data->done = true; > > +} > > + > > +/* > > + * Synchronous wrapper around bdrv_co_is_allocated_above(). > > + * > > + * See bdrv_co_is_allocated_above() for details. > > + */ > > +int bdrv_is_allocated_above(BlockDriverState *top, > > BlockDriverState *base, > > + int64_t sector_num, int nb_sectors, int > > *pnum) > > +{ > > + Coroutine *co; > > + BdrvCoIsAllocatedData data = { > > + .bs = top, > > + .base = base, > > + .sector_num = sector_num, > > + .nb_sectors = nb_sectors, > > + .pnum = pnum, > > + .done = false, > > + }; > > + > > + co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); > > + qemu_coroutine_enter(co, &data); > > + while (!data.done) { > > + qemu_aio_wait(); > > + } > > + return data.ret; > > +} > > + > > BlockInfoList *qmp_query_block(Error **errp) > > { > > BlockInfoList *head = NULL, *cur_item = NULL; > > diff --git a/block.h b/block.h > > index c89590d..6f39da9 100644 > > --- a/block.h > > +++ b/block.h > > @@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs, > > int64_t sector_num, int nb_sectors); > > int bdrv_has_zero_init(BlockDriverState *bs); > > int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, > > int nb_sectors, > > int *pnum); > > - > > +int bdrv_is_allocated_above(BlockDriverState* top, > > BlockDriverState *base, > > + int64_t sector_num, int nb_sectors, > > int *pnum); > > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction > > on_read_error, > > BlockErrorAction on_write_error); > > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int > > is_read); > > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > > index 39419a0..5b34896 100644 > > --- a/qemu-img-cmds.hx > > +++ b/qemu-img-cmds.hx > > @@ -27,6 +27,12 @@ STEXI > > @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} > > ETEXI > > > > +DEF("compare", img_compare, > > + "compare [-f fmt] [-g fmt] [-p] filename1 filename2") > > +STEXI > > +@item compare [-f fmt] [-g fmt] [-p] filename1 filename2 > > +ETEXI > > + > > DEF("convert", img_convert, > > "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o > > options] [-s snapshot_name] [-S sparse_size] filename > > [filename2 [...]] output_filename") > > STEXI > > diff --git a/qemu-img.c b/qemu-img.c > > index 80cfb9b..99a2f16 100644 > > --- a/qemu-img.c > > +++ b/qemu-img.c > > @@ -96,7 +96,9 @@ static void help(void) > > " '-a' applies a snapshot (revert disk to saved > > state)\n" > > " '-c' creates a snapshot\n" > > " '-d' deletes a snapshot\n" > > - " '-l' lists all snapshots in the given image\n"; > > + " '-l' lists all snapshots in the given image\n" > > + "Parameters to compare subcommand:\n" > > + " '-g' Second image format (in case it differs from > > first image)\n"; > > > > printf("%s\nSupported formats:", help_msg); > > bdrv_iterate_format(format_print, NULL); > > @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t > > *buf1, const uint8_t *buf2, int n, > > > > #define IO_BUF_SIZE (2 * 1024 * 1024) > > > > +/* > > + * Get number of sectors that can be stored in IO buffer. > > + */ > > + > > +static int64_t sectors_to_process(int64_t total, int64_t from) > > +{ > > + int64_t rv = total - from; > > + > > + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) > > + return IO_BUF_SIZE >> BDRV_SECTOR_BITS; > > + > > + return rv; > > +} > > + > > +/* > > + * Compares two images. Exit codes: > > + * > > + * 0 - Images are identical > > + * 1 - Images differ > > + * 2 - Error occured > > + */ > > + > > +static int img_compare(int argc, char **argv) > > +{ > > + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; > > + BlockDriverState *bs1, *bs2; > > + int64_t total_sectors1, total_sectors2; > > + uint8_t *buf1 = NULL, *buf2 = NULL; > > + int pnum1, pnum2; > > + int allocated1,allocated2; > > + int flags = BDRV_O_FLAGS; > > + int progress=0,ret = 0; /* return value - 0 Ident, 1 > > Different, 2 Error */ > > + int64_t total_sectors; > > + int64_t sector_num = 0; > > + int64_t nb_sectors; > > + int c, rv, pnum; > > + uint64_t bs_sectors; > > + uint64_t progress_base; > > + > > + > > + for(;;) { > > + c = getopt(argc, argv, "pf:g:"); > > + if (c == -1) { > > + break; > > + } > > + switch(c) { > > + case 'f': > > + fmt1 = optarg; > > + if (fmt2 == NULL) > > + fmt2 = optarg; > > + break; > > + case 'g': > > + fmt2 = optarg; > > + break; > > I can guess why you chose 'g', but perhaps 'F' for consistency with > qemu-img rebase? > > Also, perhaps we can add a "strict" mode (-S) that would fail if the > images are of different size, and if a sector is allocated in one but > unallocated in the other? > > And a "silent" or "quiet" mode (-s or -q, your choice) that prints > nothing, too. > Good idea with additional modes. I'm not sure if strict should fail on allocated/unallocated - you can compare sparse/non-sparce when is this highly probable. > > + case 'p': > > + progress = 1; > > + break; > > + } > > + } > > + if (optind >= argc) { > > + help(); > > + goto out3; > > + } > > + filename1 = argv[optind++]; > > + filename2 = argv[optind++]; > > + > > + /* Initialize before goto out */ > > + qemu_progress_init(progress, 2.0); > > + > > + bs1 = bdrv_new_open(filename1, fmt1, flags); > > + if (!bs1) { > > + error_report("Can't open file %s", filename1); > > + ret = 2; > > + goto out3; > > + } > > + > > + bs2 = bdrv_new_open(filename2, fmt2, flags); > > + if (!bs2) { > > + error_report("Can't open file %s:",filename2); > > + ret = 2; > > + goto out2; > > + } > > + > > + qemu_progress_print(0, 100); > > + > > + buf1 = qemu_blockalign(bs1,IO_BUF_SIZE); > > + buf2 = qemu_blockalign(bs2,IO_BUF_SIZE); > > + bdrv_get_geometry(bs1,&bs_sectors); > > + total_sectors1 = bs_sectors; > > You can make total_sectors1/2 unsigned, and avoid the assignment. > This is using similar construct as img_convert I inspire in. I will change this in v2. > > + bdrv_get_geometry(bs2,&bs_sectors); > > + total_sectors2 = bs_sectors; > > + total_sectors = total_sectors1; > > + progress_base = total_sectors; > > over_sectors = MAX(total_sectors1, total_sectors2); > total_sector = MIN(total_sectors1, total_sectors2); > over_num = over_sectors - total_sectors; > > > + if (total_sectors1 != total_sectors2) { > > Using MAX/MIN as above, you can move this part to after you checked > the > common part of the image, so that images are read sequentially. > > You can still place the test here in strict mode. With the > assignments > given above, you can do the test simply like this: > > if (over_num > 0) { > } > I want to have this check prior common part check - we do not have to check rest of the image if we know there's something in this area of disk. > > + BlockDriverState *bsover; > > + int64_t over_sectors, over_num; > > + const char *filename_over; > > + > > + if (total_sectors1 > total_sectors2) { > > + total_sectors = total_sectors2; > > + over_sectors = total_sectors1; > > + over_num = total_sectors2; > > + bsover = bs1; > > + filename_over = filename1; > > Only bsover/filename_over are needed of course. > > > + } else { > > + total_sectors = total_sectors1; > > + over_sectors = total_sectors2; > > + over_num = total_sectors1; > > + bsover = bs1; > > bsover = bs2; > > > + filename_over = filename2; > > Again, of course only bsover/filename_over are needed with the > changes > suggested above. > > > + } > > + > > + progress_base = over_sectors; > > + > > + for (;;) { > > + if ((nb_sectors = > > sectors_to_process(over_sectors,over_num)) <= 0) > > + break; > > + > > + rv = > > bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum); > > + nb_sectors = pnum; > > + if (rv) { > > + rv = bdrv_read(bsover, over_num, buf1, nb_sectors); > > + if (rv < 0) { > > + error_report("error while reading sector %" > > PRId64 " of %s:" > > + " %s",over_num, filename_over, > > strerror(-rv)); > > + ret = 2; > > + goto out; > > + } > > + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); > > + if (rv || pnum != nb_sectors) { > > + ret = 1; > > + printf("Images are different - image size > > mismatch!\n"); > > The error is not too precise. Let's print "content mismatch at > sector > 123456", and warn if the images are of different size, but we are > still > exiting with code 0. > Ok > > + goto out; > > + } > > + } > > + over_num += nb_sectors; > > + qemu_progress_print(((float) nb_sectors / > > progress_base)*100, 100); > > qemu_progress_print(nb_sectors, over_sectors); > > > + } > > + } > > + > > + for (;;) { > > + if ((nb_sectors = sectors_to_process(total_sectors, > > sector_num)) <= 0) > > + break; > > + allocated1 = bdrv_is_allocated_above(bs1, NULL, > > sector_num, nb_sectors, > > + &pnum1); > > + allocated2 = bdrv_is_allocated_above(bs2, NULL, > > sector_num, nb_sectors, > > + &pnum2); > > + if (pnum1 < pnum2) { > > + nb_sectors = pnum1; > > + } else { > > + nb_sectors = pnum2; > > + } > > + > > + if (allocated1 == allocated2) { > > + if (allocated1) { > > + rv = bdrv_read(bs1, sector_num, buf1, nb_sectors); > > + if (rv < 0) { > > + ret = 2; > > + error_report("error while reading sector %" > > PRId64 " of %s:" > > + " %s", sector_num, filename1, > > strerror(-rv)); > > + goto out; > > + } > > + rv = bdrv_read(bs2,sector_num,buf2, nb_sectors); > > + if (rv < 0) { > > + ret = 2; > > + error_report("error while reading sector %" > > PRId64 " of %s:" > > + " %s", sector_num, filename2, > > strerror(-rv)); > > + goto out; > > + } > > + rv = compare_sectors(buf1,buf2,nb_sectors,&pnum); > > + if (rv || pnum != nb_sectors) { > > No need to check pnum != nb_sectors. We have to check this as we compare more than one sector. If the first one will be same but any other will not we get 0 return value and expect whole chunk to be same. > > > + ret = 1; > > + printf("Images are different - content > > mismatch!\n"); > > Please print the sector number here. Ok. > > > + goto out; > > + } > > + } > > + } else { > > + BlockDriverState *bstmp; > > + const char* filenametmp; > > + if (allocated1) { > > + bstmp = bs1; > > + filenametmp = filename1; > > + } else { > > + bstmp = bs2; > > + filenametmp = filename2; > > + } > > + rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors); > > + if (rv < 0) { > > + ret = 2; > > + error_report("error while reading sector %" PRId64 > > " of %s: %s", > > + sector_num, filenametmp, > > strerror(-rv)); > > + goto out; > > + } > > + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); > > + if (rv || pnum != nb_sectors) { > > + ret = 1; > > + printf("Images are different - content > > mismatch!\n"); > > Please print the sector number here. > Ok. > > + goto out; > > + } > > + } > > + sector_num += nb_sectors; > > + qemu_progress_print(((float) nb_sectors / > > progress_base)*100, 100); > > qemu_progress_print(nb_sectors, over_sectors); > > Perhaps larger_sectors is a better name than over_sectors (and > larger_only_sectors instead of over_num? but I like this one a bit > less...). > > > + } > > + printf("Images are identical.\n"); > > + > > +out: > > + bdrv_delete(bs2); > > + qemu_vfree(buf1); > > + qemu_vfree(buf2); > > +out2: > > + bdrv_delete(bs1); > > +out3: > > + qemu_progress_end(); > > + return ret; > > +} > > + > > static int img_convert(int argc, char **argv) > > { > > int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, > > cluster_sectors; > > > > Only cosmetic problems overall; looks pretty good! > > Paolo > I will update patch as you commented and resend. Mirek
Il 01/08/2012 12:44, Miroslav Rezanina ha scritto: >> >> Also, perhaps we can add a "strict" mode (-S) that would fail if the >> images are of different size, and if a sector is allocated in one but >> unallocated in the other? >> >> And a "silent" or "quiet" mode (-s or -q, your choice) that prints >> nothing, too. >> > > Good idea with additional modes. I'm not sure if strict should fail on > allocated/unallocated - you can compare sparse/non-sparce when is this > highly probable. For that use case you probably know in advance that the size is the same, so you can use normal non-strict mode. Comparing allocated/unallocated on the other hand is helpful with unit tests. >> You can make total_sectors1/2 unsigned, and avoid the assignment. >> > This is using similar construct as img_convert I inspire in. I will > change this in v2. Oh, sorry then---consistency is better, please keep it as it is now. >>> > > + bdrv_get_geometry(bs2,&bs_sectors); >>> > > + total_sectors2 = bs_sectors; >>> > > + total_sectors = total_sectors1; >>> > > + progress_base = total_sectors; >> > >> > over_sectors = MAX(total_sectors1, total_sectors2); >> > total_sector = MIN(total_sectors1, total_sectors2); >> > over_num = over_sectors - total_sectors; >> > >>> > > + if (total_sectors1 != total_sectors2) { >> > >> > Using MAX/MIN as above, you can move this part to after you checked >> > the >> > common part of the image, so that images are read sequentially. >> > >> > You can still place the test here in strict mode. With the >> > assignments >> > given above, you can do the test simply like this: >> > >> > if (over_num > 0) { >> > } >> > > I want to have this check prior common part check - we do not have > to check rest of the image if we know there's something in this area > of disk. But if one of the two images is preallocated raw, or qcow2 with preallocated metadata, or your filesystem does not support is_allocated on raw images, then you're still reading all of the image just to check against zeros. Admittedly there is no single correct solution. Hopefully other reviewers will break the tie. Paolo
On Wed, Aug 1, 2012 at 11:03 AM, Miroslav Rezanina <mrezanin@redhat.com> wrote: > This patch adds compare subcommand that compares two images. Compare has following criteria: > - only data part is compared > - unallocated sectors are not read > - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest > - qemu-img returns: > - 0 if images are identical > - 1 if images differ > - 2 on error Please add qemu-img compare documentation to qemu-img.texi. > @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, > > #define IO_BUF_SIZE (2 * 1024 * 1024) > > +/* > + * Get number of sectors that can be stored in IO buffer. > + */ > + > +static int64_t sectors_to_process(int64_t total, int64_t from) > +{ > + int64_t rv = total - from; > + > + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) > + return IO_BUF_SIZE >> BDRV_SECTOR_BITS; Please use git show | scripts/checkpatch.pl - to check coding style. Stefan
On 08/01/2012 04:03 AM, Miroslav Rezanina wrote: > This patch adds compare subcommand that compares two images. Compare has following criteria: > - only data part is compared > - unallocated sectors are not read > - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest > - qemu-img returns: > - 0 if images are identical > - 1 if images differ > - 2 on error > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > +++ b/qemu-img.c > @@ -96,7 +96,9 @@ static void help(void) > " '-a' applies a snapshot (revert disk to saved state)\n" > " '-c' creates a snapshot\n" > " '-d' deletes a snapshot\n" > - " '-l' lists all snapshots in the given image\n"; > + " '-l' lists all snapshots in the given image\n" > + "Parameters to compare subcommand:\n" > + " '-g' Second image format (in case it differs from first image)\n"; As written, this sounds like: No -f, no -g => probe both -f, no -g => -f applies to both no -f, -g => probe first, use -g for second -f, -g => use given formats for both Is that really what you meant, or do we actually get: -f, no -g => -f applies to first, probe second I think both interpretations could make sense, but I'd prefer having the omission of -g imply probing the second file type regardless of the presence or absence of -f, for consistency.
Il 01/08/2012 15:21, Eric Blake ha scritto: >> > +++ b/qemu-img.c >> > @@ -96,7 +96,9 @@ static void help(void) >> > " '-a' applies a snapshot (revert disk to saved state)\n" >> > " '-c' creates a snapshot\n" >> > " '-d' deletes a snapshot\n" >> > - " '-l' lists all snapshots in the given image\n"; >> > + " '-l' lists all snapshots in the given image\n" >> > + "Parameters to compare subcommand:\n" >> > + " '-g' Second image format (in case it differs from first image)\n"; > As written, this sounds like: > > No -f, no -g => probe both > -f, no -g => -f applies to both > no -f, -g => probe first, use -g for second > -f, -g => use given formats for both > > Is that really what you meant, or do we actually get: > > -f, no -g => -f applies to first, probe second > > I think both interpretations could make sense, but I'd prefer having the > omission of -g imply probing the second file type regardless of the > presence or absence of -f, for consistency. +1 Paolo
----- Original Message ----- > From: "Eric Blake" <eblake@redhat.com> > To: "Miroslav Rezanina" <mrezanin@redhat.com> > Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com> > Sent: Wednesday, August 1, 2012 3:21:03 PM > Subject: Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img > > On 08/01/2012 04:03 AM, Miroslav Rezanina wrote: > > This patch adds compare subcommand that compares two images. > > Compare has following criteria: > > - only data part is compared > > - unallocated sectors are not read > > - in case of different image size, exceeding part of bigger disk > > has to be zeroed/unallocated to compare rest > > - qemu-img returns: > > - 0 if images are identical > > - 1 if images differ > > - 2 on error > > > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > > > > +++ b/qemu-img.c > > @@ -96,7 +96,9 @@ static void help(void) > > " '-a' applies a snapshot (revert disk to saved > > state)\n" > > " '-c' creates a snapshot\n" > > " '-d' deletes a snapshot\n" > > - " '-l' lists all snapshots in the given image\n"; > > + " '-l' lists all snapshots in the given image\n" > > + "Parameters to compare subcommand:\n" > > + " '-g' Second image format (in case it differs from > > first image)\n"; > > As written, this sounds like: > > No -f, no -g => probe both > -f, no -g => -f applies to both > no -f, -g => probe first, use -g for second > -f, -g => use given formats for both > > Is that really what you meant, or do we actually get: > Yes, this is what I meant. > -f, no -g => -f applies to first, probe second > > I think both interpretations could make sense, but I'd prefer having > the > omission of -g imply probing the second file type regardless of the > presence or absence of -f, for consistency. > I was evaluating both approach. For me only -f means 'Use this format for input'. In case of compare input is equal to both files so the -f value will be used for both of them. There's only one subcommand allowing to specify multiple input files - convert and it use -f value for all input commands. However, unlike the compare it does not allow to specify different format for different files and has variable number of input files. So I decided to use -f as specification for both files. I do not have problem with using it for first image only but prefer this handling. I can switch handling in next version in case of more votes for it. > -- > Eric Blake eblake@redhat.com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > Regards, Miroslav Rezanina
----- Original Message ----- > From: "Paolo Bonzini" <pbonzini@redhat.com> > To: "Miroslav Rezanina" <mrezanin@redhat.com> > Cc: qemu-devel@nongnu.org > Sent: Wednesday, August 1, 2012 12:22:50 PM > Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img > > > + goto out; > > + } > > + } > > + over_num += nb_sectors; > > + qemu_progress_print(((float) nb_sectors / > > progress_base)*100, 100); > > qemu_progress_print(nb_sectors, over_sectors); > > > Paolo > This won't work. qemu_progress_print takes either (current_progress,0) or (progress_delta,progress_part) where all values are in percents - values in range (0,100). If we pass (nb_sectors,over_sectors) we get 100% thanks to the formula: nb_sectors / 100 * over_sectors Mirek
Il 02/08/2012 12:06, Miroslav Rezanina ha scritto: >>> > > + qemu_progress_print(((float) nb_sectors / >>> > > progress_base)*100, 100); >> > >> > qemu_progress_print(nb_sectors, over_sectors); >> > >> > >> > Paolo >> > > This won't work. qemu_progress_print takes either (current_progress,0) or > (progress_delta,progress_part) where all values are in percents - values in > range (0,100). If we pass (nb_sectors,over_sectors) we get 100% thanks to the > formula: > > nb_sectors / 100 * over_sectors Uff, I admit I never understood qemu_progress_print. :) Paolo
diff --git a/block.c b/block.c index b38940b..919f8e9 100644 --- a/block.c +++ b/block.c @@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; + BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2414,6 +2415,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ + BdrvCoIsAllocatedData *data = opaque; + BlockDriverState *top = data->bs; + BlockDriverState *base = data->base; + + data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num, + data->nb_sectors,data->pnum); + data->done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ + Coroutine *co; + BdrvCoIsAllocatedData data = { + .bs = top, + .base = base, + .sector_num = sector_num, + .nb_sectors = nb_sectors, + .pnum = pnum, + .done = false, + }; + + co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); + qemu_coroutine_enter(co, &data); + while (!data.done) { + qemu_aio_wait(); + } + return data.ret; +} + BlockInfoList *qmp_query_block(Error **errp) { BlockInfoList *head = NULL, *cur_item = NULL; diff --git a/block.h b/block.h index c89590d..6f39da9 100644 --- a/block.h +++ b/block.h @@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); - +int bdrv_is_allocated_above(BlockDriverState* top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, BlockErrorAction on_write_error); BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 39419a0..5b34896 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF("compare", img_compare, + "compare [-f fmt] [-g fmt] [-p] filename1 filename2") +STEXI +@item compare [-f fmt] [-g fmt] [-p] filename1 filename2 +ETEXI + DEF("convert", img_convert, "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename") STEXI diff --git a/qemu-img.c b/qemu-img.c index 80cfb9b..99a2f16 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -96,7 +96,9 @@ static void help(void) " '-a' applies a snapshot (revert disk to saved state)\n" " '-c' creates a snapshot\n" " '-d' deletes a snapshot\n" - " '-l' lists all snapshots in the given image\n"; + " '-l' lists all snapshots in the given image\n" + "Parameters to compare subcommand:\n" + " '-g' Second image format (in case it differs from first image)\n"; printf("%s\nSupported formats:", help_msg); bdrv_iterate_format(format_print, NULL); @@ -652,6 +654,223 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +/* + * Get number of sectors that can be stored in IO buffer. + */ + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ + int64_t rv = total - from; + + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) + return IO_BUF_SIZE >> BDRV_SECTOR_BITS; + + return rv; +} + +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 2 - Error occured + */ + +static int img_compare(int argc, char **argv) +{ + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; + BlockDriverState *bs1, *bs2; + int64_t total_sectors1, total_sectors2; + uint8_t *buf1 = NULL, *buf2 = NULL; + int pnum1, pnum2; + int allocated1,allocated2; + int flags = BDRV_O_FLAGS; + int progress=0,ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */ + int64_t total_sectors; + int64_t sector_num = 0; + int64_t nb_sectors; + int c, rv, pnum; + uint64_t bs_sectors; + uint64_t progress_base; + + + for(;;) { + c = getopt(argc, argv, "pf:g:"); + if (c == -1) { + break; + } + switch(c) { + case 'f': + fmt1 = optarg; + if (fmt2 == NULL) + fmt2 = optarg; + break; + case 'g': + fmt2 = optarg; + break; + case 'p': + progress = 1; + break; + } + } + if (optind >= argc) { + help(); + goto out3; + } + filename1 = argv[optind++]; + filename2 = argv[optind++]; + + /* Initialize before goto out */ + qemu_progress_init(progress, 2.0); + + bs1 = bdrv_new_open(filename1, fmt1, flags); + if (!bs1) { + error_report("Can't open file %s", filename1); + ret = 2; + goto out3; + } + + bs2 = bdrv_new_open(filename2, fmt2, flags); + if (!bs2) { + error_report("Can't open file %s:",filename2); + ret = 2; + goto out2; + } + + qemu_progress_print(0, 100); + + buf1 = qemu_blockalign(bs1,IO_BUF_SIZE); + buf2 = qemu_blockalign(bs2,IO_BUF_SIZE); + bdrv_get_geometry(bs1,&bs_sectors); + total_sectors1 = bs_sectors; + bdrv_get_geometry(bs2,&bs_sectors); + total_sectors2 = bs_sectors; + total_sectors = total_sectors1; + progress_base = total_sectors; + + if (total_sectors1 != total_sectors2) { + BlockDriverState *bsover; + int64_t over_sectors, over_num; + const char *filename_over; + + if (total_sectors1 > total_sectors2) { + total_sectors = total_sectors2; + over_sectors = total_sectors1; + over_num = total_sectors2; + bsover = bs1; + filename_over = filename1; + } else { + total_sectors = total_sectors1; + over_sectors = total_sectors2; + over_num = total_sectors1; + bsover = bs1; + filename_over = filename2; + } + + progress_base = over_sectors; + + for (;;) { + if ((nb_sectors = sectors_to_process(over_sectors,over_num)) <= 0) + break; + + rv = bdrv_is_allocated(bsover,over_num,nb_sectors,&pnum); + nb_sectors = pnum; + if (rv) { + rv = bdrv_read(bsover, over_num, buf1, nb_sectors); + if (rv < 0) { + error_report("error while reading sector %" PRId64 " of %s:" + " %s",over_num, filename_over, strerror(-rv)); + ret = 2; + goto out; + } + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); + if (rv || pnum != nb_sectors) { + ret = 1; + printf("Images are different - image size mismatch!\n"); + goto out; + } + } + over_num += nb_sectors; + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100); + } + } + + for (;;) { + if ((nb_sectors = sectors_to_process(total_sectors, sector_num)) <= 0) + break; + allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors, + &pnum1); + allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors, + &pnum2); + if (pnum1 < pnum2) { + nb_sectors = pnum1; + } else { + nb_sectors = pnum2; + } + + if (allocated1 == allocated2) { + if (allocated1) { + rv = bdrv_read(bs1, sector_num, buf1, nb_sectors); + if (rv < 0) { + ret = 2; + error_report("error while reading sector %" PRId64 " of %s:" + " %s", sector_num, filename1, strerror(-rv)); + goto out; + } + rv = bdrv_read(bs2,sector_num,buf2, nb_sectors); + if (rv < 0) { + ret = 2; + error_report("error while reading sector %" PRId64 " of %s:" + " %s", sector_num, filename2, strerror(-rv)); + goto out; + } + rv = compare_sectors(buf1,buf2,nb_sectors,&pnum); + if (rv || pnum != nb_sectors) { + ret = 1; + printf("Images are different - content mismatch!\n"); + goto out; + } + } + } else { + BlockDriverState *bstmp; + const char* filenametmp; + if (allocated1) { + bstmp = bs1; + filenametmp = filename1; + } else { + bstmp = bs2; + filenametmp = filename2; + } + rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors); + if (rv < 0) { + ret = 2; + error_report("error while reading sector %" PRId64 " of %s: %s", + sector_num, filenametmp, strerror(-rv)); + goto out; + } + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); + if (rv || pnum != nb_sectors) { + ret = 1; + printf("Images are different - content mismatch!\n"); + goto out; + } + } + sector_num += nb_sectors; + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100); + } + printf("Images are identical.\n"); + +out: + bdrv_delete(bs2); + qemu_vfree(buf1); + qemu_vfree(buf2); +out2: + bdrv_delete(bs1); +out3: + qemu_progress_end(); + return ret; +} + static int img_convert(int argc, char **argv) { int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
This patch adds compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> Patch: --