* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:03 ` [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img Miroslav Rezanina
@ 2012-08-01 10:22 ` Paolo Bonzini
2012-08-01 10:44 ` Miroslav Rezanina
2012-08-02 10:06 ` Miroslav Rezanina
2012-08-01 12:22 ` Stefan Hajnoczi
` (2 subsequent siblings)
3 siblings, 2 replies; 24+ messages in thread
From: Paolo Bonzini @ 2012-08-01 10:22 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:22 ` Paolo Bonzini
@ 2012-08-01 10:44 ` Miroslav Rezanina
2012-08-01 10:52 ` Paolo Bonzini
2012-08-02 10:06 ` Miroslav Rezanina
1 sibling, 1 reply; 24+ messages in thread
From: Miroslav Rezanina @ 2012-08-01 10:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
----- 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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:44 ` Miroslav Rezanina
@ 2012-08-01 10:52 ` Paolo Bonzini
0 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2012-08-01 10:52 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:22 ` Paolo Bonzini
2012-08-01 10:44 ` Miroslav Rezanina
@ 2012-08-02 10:06 ` Miroslav Rezanina
2012-08-02 11:11 ` Paolo Bonzini
1 sibling, 1 reply; 24+ messages in thread
From: Miroslav Rezanina @ 2012-08-02 10:06 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
----- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-02 10:06 ` Miroslav Rezanina
@ 2012-08-02 11:11 ` Paolo Bonzini
0 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2012-08-02 11:11 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:03 ` [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img Miroslav Rezanina
2012-08-01 10:22 ` Paolo Bonzini
@ 2012-08-01 12:22 ` Stefan Hajnoczi
2012-08-01 13:21 ` Eric Blake
2012-08-03 6:45 ` [Qemu-devel] [PATCH v2][RFC] " Miroslav Rezanina
3 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2012-08-01 12:22 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 10:03 ` [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img Miroslav Rezanina
2012-08-01 10:22 ` Paolo Bonzini
2012-08-01 12:22 ` Stefan Hajnoczi
@ 2012-08-01 13:21 ` Eric Blake
2012-08-01 13:23 ` Paolo Bonzini
2012-08-02 5:19 ` Miroslav Rezanina
2012-08-03 6:45 ` [Qemu-devel] [PATCH v2][RFC] " Miroslav Rezanina
3 siblings, 2 replies; 24+ messages in thread
From: Eric Blake @ 2012-08-01 13:21 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Paolo Bonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]
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.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 13:21 ` Eric Blake
@ 2012-08-01 13:23 ` Paolo Bonzini
2012-08-02 5:19 ` Miroslav Rezanina
1 sibling, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2012-08-01 13:23 UTC (permalink / raw)
To: Eric Blake; +Cc: Miroslav Rezanina, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
2012-08-01 13:21 ` Eric Blake
2012-08-01 13:23 ` Paolo Bonzini
@ 2012-08-02 5:19 ` Miroslav Rezanina
1 sibling, 0 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-08-02 5:19 UTC (permalink / raw)
To: Eric Blake; +Cc: Paolo Bonzini, qemu-devel
----- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
2012-08-01 10:03 ` [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img Miroslav Rezanina
` (2 preceding siblings ...)
2012-08-01 13:21 ` Eric Blake
@ 2012-08-03 6:45 ` Miroslav Rezanina
2012-08-03 15:23 ` Eric Blake
` (2 more replies)
3 siblings, 3 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-08-03 6:45 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
This is second version of patch adding compare subcommand that compares two
images. Compare has following criteria:
- only data part is compared
- unallocated sectors are not read
- in case of different image size, exceeding part of bigger disk has
to be zeroed/unallocated to compare rest
- qemu-img returns:
- 0 if images are identical
- 1 if images differ
- 2 on error
v2:
- changed option for second image format to -F
- changed handlig of -f and -F [1]
- added strict mode (-s)
- added quiet mode (-q)
- improved output messages [2]
- rename variables for larger image handling
- added man page content
[1] Original patch handling was as following:
i) neither -f nor -F - both images probed for type
ii) -f only - both images use specified type
iii) -F only - first image probed, second image use specified type
iii) -f and -F - first image use -f type, second use -F type
This patch change behavior in way that case ii) and iii) has same efect - we
use specified value for both images.
[2] When we hit different sector we print its number out.
Points to dicuss:
i) Handling -f/-F options.
Currently we have three scenarios - no option
specified - probe all, one of options specified - use it for both, both option
specified - use each value for related image. This behavior is based on idea
that we can use format probing for all images or specify format for all images.
This preserve state when -f fmt specify input image format (compare is only
subcomand with more than one input image except convert that uses multiple
images without possibility to specify different format for each image).
However, there's one more behavior to be considered - to use -f/-F for one
image only - when only one option is provided, only appropriate image use specified
format, second one is probed.
ii) How to handle images with different size.
If size of images is different and strict mode is not used, addditional size of
bigger image is checked to be zeroed/unallocated. This version do this check
before rest of image is compared. This is done to not compare whole image in
case that one of images is only expanded copy of other.
Paolo Bonzini proposed to do this check after compare shared size of images to
go through image sequentially.
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
block.c | 39 ++++++++
block.h | 3 +-
qemu-img-cmds.hx | 6 +
qemu-img.c | 277 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
qemu-img.texi | 33 +++++++
5 files changed, 356 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index b38940b..3558bf9 100644
--- a/block.c
+++ b/block.c
@@ -2284,6 +2284,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
typedef struct BdrvCoIsAllocatedData {
BlockDriverState *bs;
+ BlockDriverState *base;
int64_t sector_num;
int nb_sectors;
int *pnum;
@@ -2414,6 +2415,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
return 0;
}
+/* Coroutine wrapper for bdrv_is_allocated_above() */
+static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
+{
+ BdrvCoIsAllocatedData *data = opaque;
+ BlockDriverState *top = data->bs;
+ BlockDriverState *base = data->base;
+
+ data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
+ data->nb_sectors, data->pnum);
+ data->done = true;
+}
+
+/*
+ * Synchronous wrapper around bdrv_co_is_allocated_above().
+ *
+ * See bdrv_co_is_allocated_above() for details.
+ */
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+ int64_t sector_num, int nb_sectors, int *pnum)
+{
+ Coroutine *co;
+ BdrvCoIsAllocatedData data = {
+ .bs = top,
+ .base = base,
+ .sector_num = sector_num,
+ .nb_sectors = nb_sectors,
+ .pnum = pnum,
+ .done = false,
+ };
+
+ co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
+ qemu_coroutine_enter(co, &data);
+ while (!data.done) {
+ qemu_aio_wait();
+ }
+ return data.ret;
+}
+
BlockInfoList *qmp_query_block(Error **errp)
{
BlockInfoList *head = NULL, *cur_item = NULL;
diff --git a/block.h b/block.h
index c89590d..e520eec 100644
--- a/block.h
+++ b/block.h
@@ -256,7 +256,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
int bdrv_has_zero_init(BlockDriverState *bs);
int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
int *pnum);
-
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+ int64_t sector_num, int nb_sectors, int *pnum);
void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
BlockErrorAction on_write_error);
BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 39419a0..7ee0f69 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -27,6 +27,12 @@ STEXI
@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
ETEXI
+DEF("compare", img_compare,
+ "compare [-f fmt] [-g fmt] [-p] filename1 filename2")
+STEXI
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+ETEXI
+
DEF("convert", img_convert,
"convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
STEXI
diff --git a/qemu-img.c b/qemu-img.c
index 80cfb9b..6722fa0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -96,7 +96,11 @@ static void help(void)
" '-a' applies a snapshot (revert disk to saved state)\n"
" '-c' creates a snapshot\n"
" '-d' deletes a snapshot\n"
- " '-l' lists all snapshots in the given image\n";
+ " '-l' lists all snapshots in the given image\n"
+ "Parameters to compare subcommand:\n"
+ " '-F' Second image format (in case it differs from first image)\n"
+ " '-q' Quiet mode - do not print any output (except errors)\n"
+ " '-s' Strict mode - fail on different image size or sector allocation\n";
printf("%s\nSupported formats:", help_msg);
bdrv_iterate_format(format_print, NULL);
@@ -652,6 +656,277 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
#define IO_BUF_SIZE (2 * 1024 * 1024)
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+ int64_t rv = total - from;
+
+ if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
+ return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
+ }
+
+ return rv;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occured
+ */
+
+static int img_compare(int argc, char **argv)
+{
+ const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+ BlockDriverState *bs1, *bs2;
+ int64_t total_sectors1, total_sectors2;
+ uint8_t *buf1 = NULL, *buf2 = NULL;
+ int pnum1, pnum2;
+ int allocated1, allocated2;
+ int flags = BDRV_O_FLAGS;
+ int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+ int progress = 0, quiet = 0, strict = 0;
+ int64_t total_sectors;
+ int64_t sector_num = 0;
+ int64_t nb_sectors;
+ int c, rv, pnum;
+ uint64_t bs_sectors;
+ uint64_t progress_base;
+
+
+ for (;;) {
+ c = getopt(argc, argv, "pf:F:sq");
+ if (c == -1) {
+ break;
+ }
+ switch (c) {
+ case 'f':
+ fmt1 = optarg;
+ if (fmt2 == NULL) {
+ fmt2 = optarg;
+ }
+ break;
+ case 'F':
+ fmt2 = optarg;
+ if (fmt1 == NULL) {
+ fmt2 = optarg;
+ }
+ break;
+ case 'p':
+ progress = (quiet == 0) ? 1 : 0;
+ break;
+ case 'q':
+ quiet = 1;
+ if (progress == 1) {
+ progress = 0;
+ }
+ break;
+ case 's':
+ strict = 1;
+ break;
+ }
+ }
+ if (optind >= argc) {
+ help();
+ goto out3;
+ }
+ filename1 = argv[optind++];
+ filename2 = argv[optind++];
+
+ /* Initialize before goto out */
+ qemu_progress_init(progress, 2.0);
+
+ bs1 = bdrv_new_open(filename1, fmt1, flags);
+ if (!bs1) {
+ error_report("Can't open file %s", filename1);
+ ret = 2;
+ goto out3;
+ }
+
+ bs2 = bdrv_new_open(filename2, fmt2, flags);
+ if (!bs2) {
+ error_report("Can't open file %s:", filename2);
+ ret = 2;
+ goto out2;
+ }
+
+ buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
+ buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
+ bdrv_get_geometry(bs1, &bs_sectors);
+ total_sectors1 = bs_sectors;
+ bdrv_get_geometry(bs2, &bs_sectors);
+ total_sectors2 = bs_sectors;
+ total_sectors = total_sectors1;
+ progress_base = total_sectors;
+
+ qemu_progress_print(0, 100);
+
+ if (total_sectors1 != total_sectors2) {
+ BlockDriverState *bsover;
+ int64_t lo_total_sectors, lo_sector_num;
+ const char *filename_over;
+
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Image size mismatch!");
+ }
+ goto out;
+ } else if (!quiet) {
+ printf("Warning: Image size mismatch!\n");
+ }
+
+ if (total_sectors1 > total_sectors2) {
+ total_sectors = total_sectors2;
+ lo_total_sectors = total_sectors1;
+ lo_sector_num = total_sectors2;
+ bsover = bs1;
+ filename_over = filename1;
+ } else {
+ total_sectors = total_sectors1;
+ lo_total_sectors = total_sectors2;
+ lo_sector_num = total_sectors1;
+ bsover = bs2;
+ filename_over = filename2;
+ }
+
+ progress_base = lo_total_sectors;
+
+ for (;;) {
+ nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ rv = bdrv_is_allocated(bsover, lo_sector_num, nb_sectors, &pnum);
+ nb_sectors = pnum;
+ if (rv) {
+ rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ error_report("error while reading sector %" PRId64
+ " of %s: %s", lo_sector_num, filename_over,
+ strerror(-rv));
+ ret = 2;
+ goto out;
+ }
+ rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch - Sector %" PRId64
+ " not available in both images!\n",
+ rv ? lo_sector_num : lo_sector_num + pnum);
+ }
+ goto out;
+ }
+ }
+ lo_sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ }
+
+
+ for (;;) {
+ nb_sectors = sectors_to_process(total_sectors, sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+ &pnum1);
+ allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+ &pnum2);
+ if (pnum1 < pnum2) {
+ nb_sectors = pnum1;
+ } else {
+ nb_sectors = pnum2;
+ }
+
+ if (allocated1 == allocated2) {
+ if (allocated1) {
+ rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading sector %" PRId64 " of %s:"
+ " %s", sector_num, filename1, strerror(-rv));
+ goto out;
+ }
+ rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading sector %" PRId64
+ " of %s: %s", sector_num, filename2,
+ strerror(-rv));
+ goto out;
+ }
+ rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at sector %" PRId64 "!\n",
+ rv ? sector_num : sector_num + pnum);
+ }
+ goto out;
+ }
+ }
+ } else {
+ BlockDriverState *bstmp;
+ const char *filenametmp;
+
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Sector %" PRId64
+ " allocation mismatch!",
+ sector_num);
+ }
+ goto out;
+ }
+
+ if (allocated1) {
+ bstmp = bs1;
+ filenametmp = filename1;
+ } else {
+ bstmp = bs2;
+ filenametmp = filename2;
+ }
+ rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading sector %" PRId64 " of %s: %s",
+ sector_num, filenametmp, strerror(-rv));
+ goto out;
+ }
+ rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at sector %" PRId64 "!\n",
+ rv ? sector_num : sector_num + pnum);
+ }
+ goto out;
+ }
+ }
+ sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ if (!quiet) {
+ printf("Images are identical.\n");
+ }
+
+out:
+ bdrv_delete(bs2);
+ qemu_vfree(buf1);
+ qemu_vfree(buf2);
+out2:
+ bdrv_delete(bs1);
+out3:
+ qemu_progress_end();
+ return ret;
+}
+
static int img_convert(int argc, char **argv)
{
int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
diff --git a/qemu-img.texi b/qemu-img.texi
index 77c6d0b..2972118 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -67,6 +67,18 @@ deletes a snapshot
lists all snapshots in the given image
@end table
+Parameters to compare subcommand:
+
+@table @option
+
+@item -F
+Second image format (in case it differs from first image)
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
Command description:
@table @option
@@ -100,6 +112,27 @@ it doesn't need to be specified separately in this case.
Commit the changes recorded in @var{filename} in its base image.
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Compare content of two images. You can compare images with different format or
+settings.
+
+Format is probed unless you specify it by @var{-f} and/or @var{-F} option.
+If only one of these options is specified, it is used for both images.
+If both options are specfied, @var{-f} is used for @var{filename1} and
+@var{-F} for @var{filename2}.
+
+By default, compare evaluate as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
--
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
2012-08-03 6:45 ` [Qemu-devel] [PATCH v2][RFC] " Miroslav Rezanina
@ 2012-08-03 15:23 ` Eric Blake
2012-11-20 12:36 ` Kevin Wolf
2012-11-21 9:50 ` [Qemu-devel] [PATCH v3] " Miroslav Rezanina
2 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2012-08-03 15:23 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]
On 08/03/2012 12:45 AM, Miroslav Rezanina wrote:
> This is second version of patch adding compare subcommand that compares two
> images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has
> to be zeroed/unallocated to compare rest
> - qemu-img returns:
> - 0 if images are identical
> - 1 if images differ
> - 2 on error
>
> v2:
> - changed option for second image format to -F
> - changed handlig of -f and -F [1]
> - added strict mode (-s)
> - added quiet mode (-q)
> - improved output messages [2]
> - rename variables for larger image handling
> - added man page content
>
> [1] Original patch handling was as following:
> i) neither -f nor -F - both images probed for type
> ii) -f only - both images use specified type
> iii) -F only - first image probed, second image use specified type
> iii) -f and -F - first image use -f type, second use -F type
>
> This patch change behavior in way that case ii) and iii) has same efect - we
> use specified value for both images.
I still think orthogonality is better than applying one option to both
files. Probing is sometimes useful, and you have left no way to probe
one file but not the other.
>
> [2] When we hit different sector we print its number out.
>
> Points to dicuss:
>
> i) Handling -f/-F options.
> Currently we have three scenarios - no option
> specified - probe all, one of options specified - use it for both, both option
> specified - use each value for related image. This behavior is based on idea
> that we can use format probing for all images or specify format for all images.
> This preserve state when -f fmt specify input image format (compare is only
> subcomand with more than one input image except convert that uses multiple
> images without possibility to specify different format for each image).
>
> However, there's one more behavior to be considered - to use -f/-F for one
> image only - when only one option is provided, only appropriate image use specified
> format, second one is probed.
I would prefer this, as it would let me compare against a file of
unknown type.
> +++ b/qemu-img-cmds.hx
> @@ -27,6 +27,12 @@ STEXI
> @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
> ETEXI
>
> +DEF("compare", img_compare,
> + "compare [-f fmt] [-g fmt] [-p] filename1 filename2")
Out of date with the rest of your patch.
> +STEXI
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2}
> +ETEXI
> +
> DEF("convert", img_convert,
> "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
> STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 80cfb9b..6722fa0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -96,7 +96,11 @@ static void help(void)
> " '-a' applies a snapshot (revert disk to saved state)\n"
> " '-c' creates a snapshot\n"
> " '-d' deletes a snapshot\n"
> - " '-l' lists all snapshots in the given image\n";
> + " '-l' lists all snapshots in the given image\n"
> + "Parameters to compare subcommand:\n"
> + " '-F' Second image format (in case it differs from first image)\n"
If you make -f and -F orthogonal, applying to one image each, this might
be better worded as:
'-F' Second image format (-f applies only to first image)\n
or even just
'-F' Second image format
> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occured
s/occured/occurred/
> +++ b/qemu-img.texi
> @@ -67,6 +67,18 @@ deletes a snapshot
> lists all snapshots in the given image
> @end table
>
> +Parameters to compare subcommand:
> +
> +@table @option
> +
> +@item -F
> +Second image format (in case it differs from first image)
Another instance of wording to be careful of.
> @@ -100,6 +112,27 @@ it doesn't need to be specified separately in this case.
>
> Commit the changes recorded in @var{filename} in its base image.
>
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
> +
> +Compare content of two images. You can compare images with different format or
> +settings.
> +
> +Format is probed unless you specify it by @var{-f} and/or @var{-F} option.
> +If only one of these options is specified, it is used for both images.
> +If both options are specfied, @var{-f} is used for @var{filename1} and
> +@var{-F} for @var{filename2}.
> +
> +By default, compare evaluate as identical images with different size where
s/evaluate/evaluates/
> +bigger image contains only unallocated and/or zeroed sectors in area above
> +second image size. In addition, if any sector is not allocated in one image
> +and contains only zero bytes in second, it is evaluated as equal. You can use
> +Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
> +it fails in case image size differs or sector is allocated in one image and
> +is not allocated in second.
> +
> +In case you want to suppress any non-error output, you can use Quiet mode by
> +specifying @var{-q} option.
When -q is not in use, what gets output? Is it like cmp(1), where
output is silent on the same, and lists the location of the first
differing byte on different?
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
2012-08-03 6:45 ` [Qemu-devel] [PATCH v2][RFC] " Miroslav Rezanina
2012-08-03 15:23 ` Eric Blake
@ 2012-11-20 12:36 ` Kevin Wolf
2012-11-20 13:04 ` Miroslav Rezanina
2012-11-21 9:50 ` [Qemu-devel] [PATCH v3] " Miroslav Rezanina
2 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2012-11-20 12:36 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: pbonzini, qemu-devel
Am 03.08.2012 08:45, schrieb Miroslav Rezanina:
> This is second version of patch adding compare subcommand that compares two
> images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has
> to be zeroed/unallocated to compare rest
> - qemu-img returns:
> - 0 if images are identical
> - 1 if images differ
> - 2 on error
>
> v2:
> - changed option for second image format to -F
> - changed handlig of -f and -F [1]
> - added strict mode (-s)
> - added quiet mode (-q)
> - improved output messages [2]
> - rename variables for larger image handling
> - added man page content
>
> [1] Original patch handling was as following:
> i) neither -f nor -F - both images probed for type
> ii) -f only - both images use specified type
> iii) -F only - first image probed, second image use specified type
> iii) -f and -F - first image use -f type, second use -F type
>
> This patch change behavior in way that case ii) and iii) has same efect - we
> use specified value for both images.
>
> [2] When we hit different sector we print its number out.
>
> Points to dicuss:
>
> i) Handling -f/-F options.
> Currently we have three scenarios - no option
> specified - probe all, one of options specified - use it for both, both option
> specified - use each value for related image. This behavior is based on idea
> that we can use format probing for all images or specify format for all images.
> This preserve state when -f fmt specify input image format (compare is only
> subcomand with more than one input image except convert that uses multiple
> images without possibility to specify different format for each image).
>
> However, there's one more behavior to be considered - to use -f/-F for one
> image only - when only one option is provided, only appropriate image use specified
> format, second one is probed.
Like Eric, I would prefer this alternative behaviour, so that one option
is clearly related to only one image.
> ii) How to handle images with different size.
> If size of images is different and strict mode is not used, addditional size of
> bigger image is checked to be zeroed/unallocated. This version do this check
> before rest of image is compared. This is done to not compare whole image in
> case that one of images is only expanded copy of other.
>
> Paolo Bonzini proposed to do this check after compare shared size of images to
> go through image sequentially.
I think the expected semantics is that the tool doesn't print the offset
of just any difference, but of the first one. So I'd agree with Paolo here.
> + qemu_progress_print(0, 100);
> +
> + if (total_sectors1 != total_sectors2) {
> + BlockDriverState *bsover;
> + int64_t lo_total_sectors, lo_sector_num;
> + const char *filename_over;
> +
> + if (strict) {
> + ret = 1;
> + if (!quiet) {
> + printf("Strict mode: Image size mismatch!");
Missing \n?
I also wonder if it would make sense to implement something like a
qprintf(bool quiet, const char* fmt, ...) so that you don't have this
verbose if (!quiet) around each message.
Also, shouldn't this one be on stderr and be displayed even with -q?
> + }
> + goto out;
> + } else if (!quiet) {
> + printf("Warning: Image size mismatch!\n");
> + }
> +
> + if (total_sectors1 > total_sectors2) {
> + total_sectors = total_sectors2;
> + lo_total_sectors = total_sectors1;
> + lo_sector_num = total_sectors2;
> + bsover = bs1;
> + filename_over = filename1;
> + } else {
> + total_sectors = total_sectors1;
> + lo_total_sectors = total_sectors2;
> + lo_sector_num = total_sectors1;
> + bsover = bs2;
> + filename_over = filename2;
> + }
> +
> + progress_base = lo_total_sectors;
> +
> + for (;;) {
> + nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
> + if (nb_sectors <= 0) {
> + break;
> + }
> + rv = bdrv_is_allocated(bsover, lo_sector_num, nb_sectors, &pnum);
> + nb_sectors = pnum;
> + if (rv) {
> + rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
> + if (rv < 0) {
> + error_report("error while reading sector %" PRId64
> + " of %s: %s", lo_sector_num, filename_over,
> + strerror(-rv));
Please change the unit of all offsets from sectors to bytes, it's much
friendlier if you don't have to know that qemu uses an arbitrary unit of
512 bytes internally.
> + ret = 2;
> + goto out;
> + }
> + rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> + if (rv || pnum != nb_sectors) {
> + ret = 1;
> + if (!quiet) {
> + printf("Content mismatch - Sector %" PRId64
> + " not available in both images!\n",
> + rv ? lo_sector_num : lo_sector_num + pnum);
Same here, plus stderr and display even with -q? (More instances follow,
won't comment on each)
> + }
> + goto out;
> + }
> + }
> + lo_sector_num += nb_sectors;
> + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
> + }
> + }
> +
> +
> + for (;;) {
> + nb_sectors = sectors_to_process(total_sectors, sector_num);
> + if (nb_sectors <= 0) {
> + break;
> + }
> + allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
> + &pnum1);
> + allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
> + &pnum2);
> + if (pnum1 < pnum2) {
> + nb_sectors = pnum1;
> + } else {
> + nb_sectors = pnum2;
> + }
> +
> + if (allocated1 == allocated2) {
> + if (allocated1) {
> + rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
> + if (rv < 0) {
> + ret = 2;
> + error_report("error while reading sector %" PRId64 " of %s:"
> + " %s", sector_num, filename1, strerror(-rv));
> + goto out;
> + }
> + rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
> + if (rv < 0) {
> + ret = 2;
> + error_report("error while reading sector %" PRId64
> + " of %s: %s", sector_num, filename2,
> + strerror(-rv));
> + goto out;
> + }
> + rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
> + if (rv || pnum != nb_sectors) {
> + ret = 1;
> + if (!quiet) {
> + printf("Content mismatch at sector %" PRId64 "!\n",
> + rv ? sector_num : sector_num + pnum);
Same here.
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
2012-11-20 12:36 ` Kevin Wolf
@ 2012-11-20 13:04 ` Miroslav Rezanina
0 siblings, 0 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-11-20 13:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, pbonzini
----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org, pbonzini@redhat.com
> 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:
> > This is second version of patch adding compare subcommand that
> > compares two
> > images. Compare has following criteria:
> > - only data part is compared
> > - unallocated sectors are not read
> > - in case of different image size, exceeding part of bigger disk
> > has
> > to be zeroed/unallocated to compare rest
> > - qemu-img returns:
> > - 0 if images are identical
> > - 1 if images differ
> > - 2 on error
> >
> > v2:
> > - changed option for second image format to -F
> > - changed handlig of -f and -F [1]
> > - added strict mode (-s)
> > - added quiet mode (-q)
> > - improved output messages [2]
> > - rename variables for larger image handling
> > - added man page content
> >
> > [1] Original patch handling was as following:
> > i) neither -f nor -F - both images probed for type
> > ii) -f only - both images use specified type
> > iii) -F only - first image probed, second image use
> > specified type
> > iii) -f and -F - first image use -f type, second use -F
> > type
> >
> > This patch change behavior in way that case ii) and iii) has same
> > efect - we
> > use specified value for both images.
> >
> > [2] When we hit different sector we print its number out.
> >
> > Points to dicuss:
> >
> > i) Handling -f/-F options.
> > Currently we have three scenarios - no option
> > specified - probe all, one of options specified - use it for both,
> > both option
> > specified - use each value for related image. This behavior is
> > based on idea
> > that we can use format probing for all images or specify format for
> > all images.
> > This preserve state when -f fmt specify input image format (compare
> > is only
> > subcomand with more than one input image except convert that uses
> > multiple
> > images without possibility to specify different format for each
> > image).
> >
> > However, there's one more behavior to be considered - to use -f/-F
> > for one
> > image only - when only one option is provided, only appropriate
> > image use specified
> > format, second one is probed.
>
> Like Eric, I would prefer this alternative behaviour, so that one
> option
> is clearly related to only one image.
>
Yeah, agree that one option affects one image only. This will be changed.
> > ii) How to handle images with different size.
> > If size of images is different and strict mode is not used,
> > addditional size of
> > bigger image is checked to be zeroed/unallocated. This version do
> > this check
> > before rest of image is compared. This is done to not compare whole
> > image in
> > case that one of images is only expanded copy of other.
> >
> > Paolo Bonzini proposed to do this check after compare shared size
> > of images to
> > go through image sequentially.
>
> I think the expected semantics is that the tool doesn't print the
> offset
> of just any difference, but of the first one. So I'd agree with Paolo
> here.
Question is - what do expected. I would expect to not even bother with content
in case images differ in size - this is done on strict mode. In default mode
different size is ignored in case it is empty. This mean that "same size or
zero additional space" is condition preceeding condition "all sectors are same".
User should get "Images differ in size" message instead of "Sector XXX mismatch".
Different size can suggest wrong images are compared, but different sector could
leads to search what change this sector in one of the images?
>
> > + 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.
Yes, this is good idea.
>
> Also, shouldn't this one be on stderr and be displayed even with -q?
I don't think so. It's not error, different images are one of possible results
we ask for - Output is: same/different (with reason in normal mode).
>
> > + }
> > + goto out;
> > + } else if (!quiet) {
> > + printf("Warning: Image size mismatch!\n");
> > + }
> > +
> > + if (total_sectors1 > total_sectors2) {
> > + total_sectors = total_sectors2;
> > + lo_total_sectors = total_sectors1;
> > + lo_sector_num = total_sectors2;
> > + bsover = bs1;
> > + filename_over = filename1;
> > + } else {
> > + total_sectors = total_sectors1;
> > + lo_total_sectors = total_sectors2;
> > + lo_sector_num = total_sectors1;
> > + bsover = bs2;
> > + filename_over = filename2;
> > + }
> > +
> > + progress_base = lo_total_sectors;
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(lo_total_sectors,
> > lo_sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + rv = bdrv_is_allocated(bsover, lo_sector_num,
> > nb_sectors, &pnum);
> > + nb_sectors = pnum;
> > + if (rv) {
> > + rv = bdrv_read(bsover, lo_sector_num, buf1,
> > nb_sectors);
> > + if (rv < 0) {
> > + error_report("error while reading sector %"
> > PRId64
> > + " of %s: %s", lo_sector_num,
> > filename_over,
> > + strerror(-rv));
>
> Please change the unit of all offsets from sectors to bytes, it's
> much
> friendlier if you don't have to know that qemu uses an arbitrary unit
> of
> 512 bytes internally.
Ok, will be changed.
>
> > + ret = 2;
> > + goto out;
> > + }
> > + rv = is_allocated_sectors(buf1, nb_sectors,
> > &pnum);
> > + if (rv || pnum != nb_sectors) {
> > + ret = 1;
> > + if (!quiet) {
> > + printf("Content mismatch - Sector %"
> > PRId64
> > + " not available in both images!\n",
> > + rv ? lo_sector_num : lo_sector_num
> > + pnum);
>
> Same here, plus stderr and display even with -q? (More instances
> follow,
> won't comment on each)
See above.
>
> > + }
> > + goto out;
> > + }
> > + }
> > + lo_sector_num += nb_sectors;
> > + qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> > + }
> > + }
> > +
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(total_sectors,
> > sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + allocated1 = bdrv_is_allocated_above(bs1, NULL,
> > sector_num, nb_sectors,
> > + &pnum1);
> > + allocated2 = bdrv_is_allocated_above(bs2, NULL,
> > sector_num, nb_sectors,
> > + &pnum2);
> > + if (pnum1 < pnum2) {
> > + nb_sectors = pnum1;
> > + } else {
> > + nb_sectors = pnum2;
> > + }
> > +
> > + if (allocated1 == allocated2) {
> > + if (allocated1) {
> > + rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
> > + if (rv < 0) {
> > + ret = 2;
> > + error_report("error while reading sector %"
> > PRId64 " of %s:"
> > + " %s", sector_num, filename1,
> > strerror(-rv));
> > + goto out;
> > + }
> > + rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
> > + if (rv < 0) {
> > + ret = 2;
> > + error_report("error while reading sector %"
> > PRId64
> > + " of %s: %s", sector_num,
> > filename2,
> > + strerror(-rv));
> > + goto out;
> > + }
> > + rv = compare_sectors(buf1, buf2, nb_sectors,
> > &pnum);
> > + if (rv || pnum != nb_sectors) {
> > + ret = 1;
> > + if (!quiet) {
> > + printf("Content mismatch at sector %"
> > PRId64 "!\n",
> > + rv ? sector_num : sector_num +
> > pnum);
>
> Same here.
>
See above
> Kevin
>
Thanks for comments,
Mirek
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v3] Add compare subcommand for qemu-img
2012-08-03 6:45 ` [Qemu-devel] [PATCH v2][RFC] " Miroslav Rezanina
2012-08-03 15:23 ` Eric Blake
2012-11-20 12:36 ` Kevin Wolf
@ 2012-11-21 9:50 ` Miroslav Rezanina
2012-11-22 9:18 ` Stefan Hajnoczi
2012-11-27 8:03 ` [Qemu-devel] [PATCH v4] " Miroslav Rezanina
2 siblings, 2 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-11-21 9:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini
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;
+ }
+
+ return rv;
+}
+
+/*
+ * 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 = (quiet == 0) ? 1 : 0;
+ break;
+ case 'q':
+ quiet = 1;
+ if (progress == 1) {
+ progress = 0;
+ }
+ break;
+ case 's':
+ strict = 1;
+ break;
+ }
+ }
+ if (optind >= argc) {
+ help();
+ goto out3;
+ }
+ filename1 = argv[optind++];
+ filename2 = argv[optind++];
+
+ /* Initialize before goto out */
+ qemu_progress_init(progress, 2.0);
+
+ bs1 = bdrv_new_open(filename1, fmt1, flags, 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 = 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);
+ 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);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch - offset %" PRId64
+ " not available in both images!\n",
+ sector_to_bytes(
+ rv ? lo_sector_num : lo_sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ lo_sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ }
+
+
+ for (;;) {
+ nb_sectors = sectors_to_process(total_sectors, sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+ &pnum1);
+ allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+ &pnum2);
+ if (pnum1 < pnum2) {
+ nb_sectors = pnum1;
+ } else {
+ nb_sectors = pnum2;
+ }
+
+ if (allocated1 == allocated2) {
+ if (allocated1) {
+ rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64 " of %s:"
+ " %s", sector_to_bytes(sector_num), filename1,
+ strerror(-rv));
+ goto out;
+ }
+ rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64
+ " of %s: %s", sector_to_bytes(sector_num),
+ filename2, strerror(-rv));
+ goto out;
+ }
+ rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sector_to_bytes(
+ rv ? sector_num : sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ } else {
+ BlockDriverState *bstmp;
+ const char *filenametmp;
+
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Offset %" PRId64
+ " allocation mismatch!",
+ sector_to_bytes(sector_num));
+ }
+ goto out;
+ }
+
+ if (allocated1) {
+ bstmp = bs1;
+ filenametmp = filename1;
+ } else {
+ bstmp = bs2;
+ filenametmp = filename2;
+ }
+ rv = bdrv_read(bstmp, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64 " of %s: %s",
+ sector_to_bytes(sector_num), filenametmp,
+ strerror(-rv));
+ goto out;
+ }
+ rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sector_to_bytes(
+ rv ? sector_num : sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ if (!quiet) {
+ printf("Images are identical.\n");
+ }
+
+out:
+ bdrv_delete(bs2);
+ qemu_vfree(buf1);
+ qemu_vfree(buf2);
+out2:
+ bdrv_delete(bs1);
+out3:
+ qemu_progress_end();
+ return ret;
+}
static int img_convert(int argc, char **argv)
{
diff --git a/qemu-img.texi b/qemu-img.texi
index 60b83fc..cc54653 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -81,6 +81,20 @@ deletes a snapshot
lists all snapshots in the given image
@end table
+Parameters to compare subcommand:
+
+@table @option
+
+@item -f
+First image format
+@item -F
+Second image format
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
Command description:
@table @option
@@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
Commit the changes recorded in @var{filename} in its base image.
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Check if two images contains same content. You can compare images with
+different format or settings.
+
+Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+
+By default, compare evaluates as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+By default, compare prints out result message. This message displays
+information that both images are same or position of first different byte.
+In addition, result message can report different image size in case Strict
+mode is used.
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v3] Add compare subcommand for qemu-img
2012-11-21 9:50 ` [Qemu-devel] [PATCH v3] " Miroslav Rezanina
@ 2012-11-22 9:18 ` Stefan Hajnoczi
2012-11-27 8:03 ` [Qemu-devel] [PATCH v4] " Miroslav Rezanina
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2012-11-22 9:18 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel
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 &pnum);
> + 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);
> + if (rv || pnum != nb_sectors) {
> + ret = 1;
> + if (!quiet) {
> + printf("Content mismatch - offset %" PRId64
> + " not available in both images!\n",
> + sector_to_bytes(
> + rv ? lo_sector_num : lo_sector_num + pnum));
> + }
> + goto out;
> + }
This is duplicated from the code below.
Please split this function up into helper functions. It will make some
of the temporary variables go away too.
^ permalink raw reply [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v4] Add compare subcommand for qemu-img
2012-11-21 9:50 ` [Qemu-devel] [PATCH v3] " Miroslav Rezanina
2012-11-22 9:18 ` Stefan Hajnoczi
@ 2012-11-27 8:03 ` Miroslav Rezanina
2012-11-30 14:22 ` Stefan Hajnoczi
2012-12-04 11:06 ` [Qemu-devel] [PATCH v5] " Miroslav Rezanina
1 sibling, 2 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-11-27 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi
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
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 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 <mrezanin@redhat.com>
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..9cc4365 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,13 @@ static void help(void)
" '-a' applies a snapshot (revert disk to saved state)\n"
" '-c' creates a snapshot\n"
" '-d' deletes a snapshot\n"
- " '-l' lists all snapshots in the given image\n";
+ " '-l' lists all snapshots in the given image\n"
+ "\n"
+ "Parameters to compare subcommand:\n"
+ " '-f' First image format\n"
+ " '-F' Second image format\n"
+ " '-q' Quiet mode - do not print any output (except errors)\n"
+ " '-s' Strict mode - fail on different image size or sector allocation\n";
printf("%s\nSupported formats:", help_msg);
bdrv_iterate_format(format_print, NULL);
@@ -658,6 +664,288 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
#define IO_BUF_SIZE (2 * 1024 * 1024)
+/*
+ * Helper functionto display offset in bytes insted of sector number.
+ */
+static int64_t sectors_to_bytes(int64_t sectors)
+{
+ return sectors << BDRV_SECTOR_BITS;
+}
+
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+ int64_t rv = total - from;
+
+ if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
+ return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
+ }
+
+ return rv;
+}
+
+/*
+ * Check if passed sectors are filled with 0.
+ *
+ * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
+ * data and negative value on error.
+ */
+static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
+ const char *filename, uint8_t *buffer, int quiet)
+{
+ int pnum, rv = 0;
+ rv = bdrv_read(bs, sect_num, buffer, sect_count);
+ if (rv < 0) {
+ error_report("Error while reading offset %" PRId64 " of %s: %s",
+ sectors_to_bytes(sect_num), filename, strerror(-rv));
+ return rv;
+ }
+ rv = is_allocated_sectors(buffer, sect_count, &pnum);
+ if (rv || pnum != sect_count) {
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(rv ? sect_num : sect_num + pnum));
+ }
+ return 1;
+ }
+
+ return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred
+ */
+static int img_compare(int argc, char **argv)
+{
+ const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+ BlockDriverState *bs1, *bs2;
+ int64_t total_sectors1, total_sectors2;
+ uint8_t *buf1 = NULL, *buf2 = NULL;
+ int pnum1, pnum2;
+ int allocated1, allocated2;
+ int flags = BDRV_O_FLAGS;
+ int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+ int progress = 0, quiet = 0, strict = 0;
+ int64_t total_sectors;
+ int64_t sector_num = 0;
+ int64_t nb_sectors;
+ int c, rv, pnum;
+ uint64_t bs_sectors;
+ uint64_t progress_base;
+
+
+ for (;;) {
+ c = getopt(argc, argv, "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;
+ }
+ }
+
+ /* Progress is not shown in Quiet mode */
+ if (quiet) {
+ progress = 0;
+ }
+
+ if (optind > argc - 2) {
+ 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);
+ 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_above(bsover, NULL, lo_sector_num,
+ nb_sectors, &pnum);
+ nb_sectors = pnum;
+ if (rv) {
+ rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
+ filename_over, buf1, quiet);
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ lo_sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ }
+
+
+ for (;;) {
+ nb_sectors = sectors_to_process(total_sectors, sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+ &pnum1);
+ allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+ &pnum2);
+ if (pnum1 < pnum2) {
+ nb_sectors = pnum1;
+ } else {
+ nb_sectors = pnum2;
+ }
+
+ if (allocated1 == allocated2) {
+ if (allocated1) {
+ rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64 " of %s:"
+ " %s", sectors_to_bytes(sector_num), filename1,
+ strerror(-rv));
+ goto out;
+ }
+ rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64
+ " of %s: %s", sectors_to_bytes(sector_num),
+ filename2, strerror(-rv));
+ goto out;
+ }
+ rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(
+ rv ? sector_num : sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ } else {
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Offset %" PRId64
+ " allocation mismatch!",
+ sectors_to_bytes(sector_num));
+ }
+ goto out;
+ }
+
+ if (allocated1) {
+ rv = is_zeroed(bs1, sector_num, nb_sectors,
+ filename1, buf1, quiet);
+ } else {
+ rv = is_zeroed(bs2, sector_num, nb_sectors,
+ filename2, buf1, quiet);
+ }
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ if (!quiet) {
+ printf("Images are identical.\n");
+ }
+
+out:
+ bdrv_delete(bs2);
+ qemu_vfree(buf1);
+ qemu_vfree(buf2);
+out2:
+ bdrv_delete(bs1);
+out3:
+ qemu_progress_end();
+ return ret;
+}
+
static int img_convert(int argc, char **argv)
{
int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
diff --git a/qemu-img.texi b/qemu-img.texi
index 60b83fc..cb073c3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -81,6 +81,20 @@ deletes a snapshot
lists all snapshots in the given image
@end table
+Parameters to compare subcommand:
+
+@table @option
+
+@item -f
+First image format
+@item -F
+Second image format
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
Command description:
@table @option
@@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
Commit the changes recorded in @var{filename} in its base image.
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Check if two images contains same content. You can compare images with
+different format or settings.
+
+Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+
+By default, compare evaluates as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+By default, compare prints out result message. This message displays
+information that both images are same or position of first different byte.
+In addition, result message can report different image size in case Strict
+mode is used.
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v4] Add compare subcommand for qemu-img
2012-11-27 8:03 ` [Qemu-devel] [PATCH v4] " Miroslav Rezanina
@ 2012-11-30 14:22 ` Stefan Hajnoczi
2012-12-04 11:06 ` [Qemu-devel] [PATCH v5] " Miroslav Rezanina
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2012-11-30 14:22 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
On Tue, Nov 27, 2012 at 9:03 AM, Miroslav Rezanina <mrezanin@redhat.com> wrote:
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..9cc4365 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,13 @@ static void help(void)
> " '-a' applies a snapshot (revert disk to saved state)\n"
> " '-c' creates a snapshot\n"
> " '-d' deletes a snapshot\n"
> - " '-l' lists all snapshots in the given image\n";
> + " '-l' lists all snapshots in the given image\n"
> + "\n"
> + "Parameters to compare subcommand:\n"
> + " '-f' First image format\n"
> + " '-F' Second image format\n"
> + " '-q' Quiet mode - do not print any output (except errors)\n"
> + " '-s' Strict mode - fail on different image size or sector allocation\n";
>
> printf("%s\nSupported formats:", help_msg);
> bdrv_iterate_format(format_print, NULL);
> @@ -658,6 +664,288 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>
> #define IO_BUF_SIZE (2 * 1024 * 1024)
>
> +/*
> + * Helper functionto display offset in bytes insted of sector number.
> + */
> +static int64_t sectors_to_bytes(int64_t sectors)
The purpose of this function is clear from its name and code. There
are typos in the doc comment. Please drop the doc comment, it's not
needed.
> +/*
> + * 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;
> + }
> + }
> +
> + /* Progress is not shown in Quiet mode */
> + if (quiet) {
> + progress = 0;
> + }
> +
> + if (optind > argc - 2) {
> + help();
> + goto out3;
help() never returns, the goto can be dropped.
> + }
> + 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 = total_sectors1;
total_sectors = MIN(total_sectors1, total_sectors2);
This way you don't need to update with it later on.
> + 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);
> + if (rv) {
> + if (rv < 0) {
> + ret = 2;
> + }
> + goto out;
> + }
> + }
> + lo_sector_num += nb_sectors;
> + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
> + }
Please move this into a separate function. It allows you to get rid
of the temporary variables:
int check_all_zeroes(BlockDriverState *bs, int64_t sector_num, int64_t
nb_sectors);
if (total_sectors1 > total_sectors2) {
rv = check_all_zeroes(bs1, total_sectors2, total_sectors2 - total_sectors1);
} else {
rv = check_all_zeroes(bs2, total_sectors1, total_sectors1 - total_sectors2);
}
^ permalink raw reply [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v5] Add compare subcommand for qemu-img
2012-11-27 8:03 ` [Qemu-devel] [PATCH v4] " Miroslav Rezanina
2012-11-30 14:22 ` Stefan Hajnoczi
@ 2012-12-04 11:06 ` Miroslav Rezanina
2012-12-04 15:22 ` Stefan Hajnoczi
2012-12-06 12:24 ` [Qemu-devel] [PATCH v6] " Miroslav Rezanina
1 sibling, 2 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-12-04 11:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi
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
v5 (only minor changes):
- removed redundant comment
- removed dead code (goto after help())
- set final total_sectors on first assignment
v4:
- Fixed various typos
- Added functions for empty sector check and sector-to-bytes offset conversion
- Fixed command-line parameters processing
v3:
- options -f/-F are orthogonal
- documentation updated to new syntax and behavior
- used byte offset instead of sector number for output
v2:
- changed option for second image format to -F
- changed handling of -f and -F [1]
- added strict mode (-s)
- added quiet mode (-q)
- improved output messages [2]
- rename variables for larger image handling
- added man page content
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
Note: Do not move any code to separate function as suggested by Stefan. Code to extract is not reusable and
require access to some of local variables (for error handling part). Only code in separate function is the
checking passed sectors for zero value as this can be reused on two places in the function.
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..6cad518 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,13 @@ static void help(void)
" '-a' applies a snapshot (revert disk to saved state)\n"
" '-c' creates a snapshot\n"
" '-d' deletes a snapshot\n"
- " '-l' lists all snapshots in the given image\n";
+ " '-l' lists all snapshots in the given image\n"
+ "\n"
+ "Parameters to compare subcommand:\n"
+ " '-f' First image format\n"
+ " '-F' Second image format\n"
+ " '-q' Quiet mode - do not print any output (except errors)\n"
+ " '-s' Strict mode - fail on different image size or sector allocation\n";
printf("%s\nSupported formats:", help_msg);
bdrv_iterate_format(format_print, NULL);
@@ -658,6 +664,282 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
#define IO_BUF_SIZE (2 * 1024 * 1024)
+static int64_t sectors_to_bytes(int64_t sectors)
+{
+ return sectors << BDRV_SECTOR_BITS;
+}
+
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+ int64_t rv = total - from;
+
+ if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
+ return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
+ }
+
+ return rv;
+}
+
+/*
+ * Check if passed sectors are filled with 0.
+ *
+ * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
+ * data and negative value on error.
+ */
+static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
+ const char *filename, uint8_t *buffer, int quiet)
+{
+ int pnum, rv = 0;
+ rv = bdrv_read(bs, sect_num, buffer, sect_count);
+ if (rv < 0) {
+ error_report("Error while reading offset %" PRId64 " of %s: %s",
+ sectors_to_bytes(sect_num), filename, strerror(-rv));
+ return rv;
+ }
+ rv = is_allocated_sectors(buffer, sect_count, &pnum);
+ if (rv || pnum != sect_count) {
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(rv ? sect_num : sect_num + pnum));
+ }
+ return 1;
+ }
+
+ return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred
+ */
+static int img_compare(int argc, char **argv)
+{
+ const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+ BlockDriverState *bs1, *bs2;
+ int64_t total_sectors1, total_sectors2;
+ uint8_t *buf1 = NULL, *buf2 = NULL;
+ int pnum1, pnum2;
+ int allocated1, allocated2;
+ int flags = BDRV_O_FLAGS;
+ int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+ int progress = 0, quiet = 0, strict = 0;
+ int64_t total_sectors;
+ int64_t sector_num = 0;
+ int64_t nb_sectors;
+ int c, rv, pnum;
+ uint64_t bs_sectors;
+ uint64_t progress_base;
+
+
+ for (;;) {
+ c = getopt(argc, argv, "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;
+ }
+ }
+
+ /* Progress is not shown in Quiet mode */
+ if (quiet) {
+ progress = 0;
+ }
+
+ if (optind > argc - 2) {
+ help();
+ }
+ filename1 = argv[optind++];
+ filename2 = argv[optind++];
+
+ /* Initialize before goto out */
+ qemu_progress_init(progress, 2.0);
+
+ bs1 = bdrv_new_open(filename1, fmt1, flags, true);
+ if (!bs1) {
+ error_report("Can't open file %s", filename1);
+ ret = 2;
+ goto out3;
+ }
+
+ bs2 = bdrv_new_open(filename2, fmt2, flags, true);
+ if (!bs2) {
+ error_report("Can't open file %s", filename2);
+ ret = 2;
+ goto out2;
+ }
+
+ buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
+ buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
+ bdrv_get_geometry(bs1, &bs_sectors);
+ total_sectors1 = bs_sectors;
+ bdrv_get_geometry(bs2, &bs_sectors);
+ total_sectors2 = bs_sectors;
+ total_sectors = MIN(total_sectors1, total_sectors2);
+ progress_base = total_sectors;
+
+ qemu_progress_print(0, 100);
+
+ if (total_sectors1 != total_sectors2) {
+ BlockDriverState *bsover;
+ int64_t lo_total_sectors, lo_sector_num;
+ const char *filename_over;
+
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Image size mismatch!\n");
+ }
+ goto out;
+ } else {
+ error_report("Image size mismatch!");
+ }
+
+ if (total_sectors1 > total_sectors2) {
+ lo_total_sectors = total_sectors1;
+ lo_sector_num = total_sectors2;
+ bsover = bs1;
+ filename_over = filename1;
+ } else {
+ lo_total_sectors = total_sectors2;
+ lo_sector_num = total_sectors1;
+ bsover = bs2;
+ filename_over = filename2;
+ }
+
+ progress_base = lo_total_sectors;
+
+ for (;;) {
+ nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num,
+ nb_sectors, &pnum);
+ nb_sectors = pnum;
+ if (rv) {
+ rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
+ filename_over, buf1, quiet);
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ lo_sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ }
+
+
+ for (;;) {
+ nb_sectors = sectors_to_process(total_sectors, sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+ &pnum1);
+ allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+ &pnum2);
+ if (pnum1 < pnum2) {
+ nb_sectors = pnum1;
+ } else {
+ nb_sectors = pnum2;
+ }
+
+ if (allocated1 == allocated2) {
+ if (allocated1) {
+ rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64 " of %s:"
+ " %s", sectors_to_bytes(sector_num), filename1,
+ strerror(-rv));
+ goto out;
+ }
+ rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64
+ " of %s: %s", sectors_to_bytes(sector_num),
+ filename2, strerror(-rv));
+ goto out;
+ }
+ rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(
+ rv ? sector_num : sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ } else {
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Offset %" PRId64
+ " allocation mismatch!",
+ sectors_to_bytes(sector_num));
+ }
+ goto out;
+ }
+
+ if (allocated1) {
+ rv = is_zeroed(bs1, sector_num, nb_sectors,
+ filename1, buf1, quiet);
+ } else {
+ rv = is_zeroed(bs2, sector_num, nb_sectors,
+ filename2, buf1, quiet);
+ }
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ if (!quiet) {
+ printf("Images are identical.\n");
+ }
+
+out:
+ bdrv_delete(bs2);
+ qemu_vfree(buf1);
+ qemu_vfree(buf2);
+out2:
+ bdrv_delete(bs1);
+out3:
+ qemu_progress_end();
+ return ret;
+}
+
static int img_convert(int argc, char **argv)
{
int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
diff --git a/qemu-img.texi b/qemu-img.texi
index 60b83fc..cb073c3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -81,6 +81,20 @@ deletes a snapshot
lists all snapshots in the given image
@end table
+Parameters to compare subcommand:
+
+@table @option
+
+@item -f
+First image format
+@item -F
+Second image format
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
Command description:
@table @option
@@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
Commit the changes recorded in @var{filename} in its base image.
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Check if two images contains same content. You can compare images with
+different format or settings.
+
+Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+
+By default, compare evaluates as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+By default, compare prints out result message. This message displays
+information that both images are same or position of first different byte.
+In addition, result message can report different image size in case Strict
+mode is used.
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v5] Add compare subcommand for qemu-img
2012-12-04 11:06 ` [Qemu-devel] [PATCH v5] " Miroslav Rezanina
@ 2012-12-04 15:22 ` Stefan Hajnoczi
2012-12-06 12:24 ` [Qemu-devel] [PATCH v6] " Miroslav Rezanina
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2012-12-04 15:22 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
On Tue, Dec 04, 2012 at 06:06:55AM -0500, Miroslav Rezanina wrote:
> + 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;
> + }
> + }
Still missing '?' and 'h'. They should call help().
Without this an invalid command-line option doesn't abort execution and
weird things can happen :).
Stefan
^ permalink raw reply [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
2012-12-04 11:06 ` [Qemu-devel] [PATCH v5] " Miroslav Rezanina
2012-12-04 15:22 ` Stefan Hajnoczi
@ 2012-12-06 12:24 ` Miroslav Rezanina
2012-12-11 9:16 ` Stefan Hajnoczi
2012-12-11 12:27 ` Kevin Wolf
1 sibling, 2 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-12-06 12:24 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi
This is second version of patch adding compare subcommand that
compares two images. Compare has following criteria:
- only data part is compared
- unallocated sectors are not read
- in case of different image size, exceeding part of bigger disk has
to be zeroed/unallocated to compare rest
- qemu-img returns:
- 0 if images are identical
- 1 if images differ
- 2 on error
v6:
- added handling -?, -h options for compare subcommand
v5 (only minor changes):
- removed redundant comment
- removed dead code (goto after help())
- set final total_sectors on first assignment
v4:
- Fixed various typos
- Added functions for empty sector check and sector-to-bytes offset
conversion
- Fixed command-line parameters processing
v3:
- options -f/-F are orthogonal
- documentation updated to new syntax and behavior
- used byte offset instead of sector number for output
v2:
- changed option for second image format to -F
- changed handling of -f and -F [1]
- added strict mode (-s)
- added quiet mode (-q)
- improved output messages [2]
- rename variables for larger image handling
- added man page content
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
diff --git a/block.c b/block.c
index 854ebd6..fdc8c45 100644
--- a/block.c
+++ b/block.c
@@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
typedef struct BdrvCoIsAllocatedData {
BlockDriverState *bs;
+ BlockDriverState *base;
int64_t sector_num;
int nb_sectors;
int *pnum;
@@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
return 0;
}
+/* Coroutine wrapper for bdrv_is_allocated_above() */
+static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
+{
+ BdrvCoIsAllocatedData *data = opaque;
+ BlockDriverState *top = data->bs;
+ BlockDriverState *base = data->base;
+
+ data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
+ data->nb_sectors, data->pnum);
+ data->done = true;
+}
+
+/*
+ * Synchronous wrapper around bdrv_co_is_allocated_above().
+ *
+ * See bdrv_co_is_allocated_above() for details.
+ */
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+ int64_t sector_num, int nb_sectors, int *pnum)
+{
+ Coroutine *co;
+ BdrvCoIsAllocatedData data = {
+ .bs = top,
+ .base = base,
+ .sector_num = sector_num,
+ .nb_sectors = nb_sectors,
+ .pnum = pnum,
+ .done = false,
+ };
+
+ co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
+ qemu_coroutine_enter(co, &data);
+ while (!data.done) {
+ qemu_aio_wait();
+ }
+ return data.ret;
+}
+
BlockInfo *bdrv_query_info(BlockDriverState *bs)
{
BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/block.h b/block.h
index 722c620..2cb8d71 100644
--- a/block.h
+++ b/block.h
@@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
int bdrv_has_zero_init(BlockDriverState *bs);
int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
int *pnum);
+int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+ int64_t sector_num, int nb_sectors, int *pnum);
void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
BlockdevOnError on_write_error);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index a181363..c076085 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -27,6 +27,12 @@ STEXI
@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
ETEXI
+DEF("compare", img_compare,
+ "compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2")
+STEXI
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+ETEXI
+
DEF("convert", img_convert,
"convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
STEXI
diff --git a/qemu-img.c b/qemu-img.c
index e29e01b..09abb3a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,13 @@ static void help(void)
" '-a' applies a snapshot (revert disk to saved state)\n"
" '-c' creates a snapshot\n"
" '-d' deletes a snapshot\n"
- " '-l' lists all snapshots in the given image\n";
+ " '-l' lists all snapshots in the given image\n"
+ "\n"
+ "Parameters to compare subcommand:\n"
+ " '-f' First image format\n"
+ " '-F' Second image format\n"
+ " '-q' Quiet mode - do not print any output (except errors)\n"
+ " '-s' Strict mode - fail on different image size or sector allocation\n";
printf("%s\nSupported formats:", help_msg);
bdrv_iterate_format(format_print, NULL);
@@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
#define IO_BUF_SIZE (2 * 1024 * 1024)
+static int64_t sectors_to_bytes(int64_t sectors)
+{
+ return sectors << BDRV_SECTOR_BITS;
+}
+
+/*
+ * Get number of sectors that can be stored in IO buffer.
+ */
+static int64_t sectors_to_process(int64_t total, int64_t from)
+{
+ int64_t rv = total - from;
+
+ if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
+ return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
+ }
+
+ return rv;
+}
+
+/*
+ * Check if passed sectors are filled with 0.
+ *
+ * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
+ * data and negative value on error.
+ */
+static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
+ const char *filename, uint8_t *buffer, int quiet)
+{
+ int pnum, rv = 0;
+ rv = bdrv_read(bs, sect_num, buffer, sect_count);
+ if (rv < 0) {
+ error_report("Error while reading offset %" PRId64 " of %s: %s",
+ sectors_to_bytes(sect_num), filename, strerror(-rv));
+ return rv;
+ }
+ rv = is_allocated_sectors(buffer, sect_count, &pnum);
+ if (rv || pnum != sect_count) {
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(rv ? sect_num : sect_num + pnum));
+ }
+ return 1;
+ }
+
+ return 0;
+}
+
+/*
+ * Compares two images. Exit codes:
+ *
+ * 0 - Images are identical
+ * 1 - Images differ
+ * 2 - Error occurred
+ */
+static int img_compare(int argc, char **argv)
+{
+ const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+ BlockDriverState *bs1, *bs2;
+ int64_t total_sectors1, total_sectors2;
+ uint8_t *buf1 = NULL, *buf2 = NULL;
+ int pnum1, pnum2;
+ int allocated1, allocated2;
+ int flags = BDRV_O_FLAGS;
+ int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
+ int progress = 0, quiet = 0, strict = 0;
+ int64_t total_sectors;
+ int64_t sector_num = 0;
+ int64_t nb_sectors;
+ int c, rv, pnum;
+ uint64_t bs_sectors;
+ uint64_t progress_base;
+
+
+ for (;;) {
+ c = getopt(argc, argv, "hpf:F:sq");
+ if (c == -1) {
+ break;
+ }
+ switch (c) {
+ case '?':
+ case 'h':
+ help();
+ break;
+ case 'f':
+ fmt1 = optarg;
+ break;
+ case 'F':
+ fmt2 = optarg;
+ break;
+ case 'p':
+ progress = 1;
+ break;
+ case 'q':
+ quiet = 1;
+ break;
+ case 's':
+ strict = 1;
+ break;
+ }
+ }
+
+ /* Progress is not shown in Quiet mode */
+ if (quiet) {
+ progress = 0;
+ }
+
+ if (optind > argc - 2) {
+ help();
+ }
+ filename1 = argv[optind++];
+ filename2 = argv[optind++];
+
+ /* Initialize before goto out */
+ qemu_progress_init(progress, 2.0);
+
+ bs1 = bdrv_new_open(filename1, fmt1, flags, true);
+ if (!bs1) {
+ error_report("Can't open file %s", filename1);
+ ret = 2;
+ goto out3;
+ }
+
+ bs2 = bdrv_new_open(filename2, fmt2, flags, true);
+ if (!bs2) {
+ error_report("Can't open file %s", filename2);
+ ret = 2;
+ goto out2;
+ }
+
+ buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
+ buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
+ bdrv_get_geometry(bs1, &bs_sectors);
+ total_sectors1 = bs_sectors;
+ bdrv_get_geometry(bs2, &bs_sectors);
+ total_sectors2 = bs_sectors;
+ total_sectors = MIN(total_sectors1, total_sectors2);
+ progress_base = total_sectors;
+
+ qemu_progress_print(0, 100);
+
+ if (total_sectors1 != total_sectors2) {
+ BlockDriverState *bsover;
+ int64_t lo_total_sectors, lo_sector_num;
+ const char *filename_over;
+
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Image size mismatch!\n");
+ }
+ goto out;
+ } else {
+ error_report("Image size mismatch!");
+ }
+
+ if (total_sectors1 > total_sectors2) {
+ lo_total_sectors = total_sectors1;
+ lo_sector_num = total_sectors2;
+ bsover = bs1;
+ filename_over = filename1;
+ } else {
+ lo_total_sectors = total_sectors2;
+ lo_sector_num = total_sectors1;
+ bsover = bs2;
+ filename_over = filename2;
+ }
+
+ progress_base = lo_total_sectors;
+
+ for (;;) {
+ nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num,
+ nb_sectors, &pnum);
+ nb_sectors = pnum;
+ if (rv) {
+ rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
+ filename_over, buf1, quiet);
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ lo_sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ }
+
+
+ for (;;) {
+ nb_sectors = sectors_to_process(total_sectors, sector_num);
+ if (nb_sectors <= 0) {
+ break;
+ }
+ allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
+ &pnum1);
+ allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
+ &pnum2);
+ if (pnum1 < pnum2) {
+ nb_sectors = pnum1;
+ } else {
+ nb_sectors = pnum2;
+ }
+
+ if (allocated1 == allocated2) {
+ if (allocated1) {
+ rv = bdrv_read(bs1, sector_num, buf1, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64 " of %s:"
+ " %s", sectors_to_bytes(sector_num), filename1,
+ strerror(-rv));
+ goto out;
+ }
+ rv = bdrv_read(bs2, sector_num, buf2, nb_sectors);
+ if (rv < 0) {
+ ret = 2;
+ error_report("error while reading offset %" PRId64
+ " of %s: %s", sectors_to_bytes(sector_num),
+ filename2, strerror(-rv));
+ goto out;
+ }
+ rv = compare_sectors(buf1, buf2, nb_sectors, &pnum);
+ if (rv || pnum != nb_sectors) {
+ ret = 1;
+ if (!quiet) {
+ printf("Content mismatch at offset %" PRId64 "!\n",
+ sectors_to_bytes(
+ rv ? sector_num : sector_num + pnum));
+ }
+ goto out;
+ }
+ }
+ } else {
+ if (strict) {
+ ret = 1;
+ if (!quiet) {
+ printf("Strict mode: Offset %" PRId64
+ " allocation mismatch!",
+ sectors_to_bytes(sector_num));
+ }
+ goto out;
+ }
+
+ if (allocated1) {
+ rv = is_zeroed(bs1, sector_num, nb_sectors,
+ filename1, buf1, quiet);
+ } else {
+ rv = is_zeroed(bs2, sector_num, nb_sectors,
+ filename2, buf1, quiet);
+ }
+ if (rv) {
+ if (rv < 0) {
+ ret = 2;
+ }
+ goto out;
+ }
+ }
+ sector_num += nb_sectors;
+ qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ }
+ if (!quiet) {
+ printf("Images are identical.\n");
+ }
+
+out:
+ bdrv_delete(bs2);
+ qemu_vfree(buf1);
+ qemu_vfree(buf2);
+out2:
+ bdrv_delete(bs1);
+out3:
+ qemu_progress_end();
+ return ret;
+}
+
static int img_convert(int argc, char **argv)
{
int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
diff --git a/qemu-img.texi b/qemu-img.texi
index 60b83fc..cb073c3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -81,6 +81,20 @@ deletes a snapshot
lists all snapshots in the given image
@end table:
+Parameters to compare subcommand:
+
+@table @option
+
+@item -f
+First image format
+@item -F
+Second image format
+@item -q
+Quiet mode - do not print any output (except errors)
+@item -s
+Strict mode - fail on on different image size or sector allocation
+@end table
+
Command description:
@table @option
@@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
Commit the changes recorded in @var{filename} in its base image.
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+
+Check if two images contains same content. You can compare images with
+different format or settings.
+
+Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+
+By default, compare evaluates as identical images with different size where
+bigger image contains only unallocated and/or zeroed sectors in area above
+second image size. In addition, if any sector is not allocated in one image
+and contains only zero bytes in second, it is evaluated as equal. You can use
+Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
+it fails in case image size differs or sector is allocated in one image and
+is not allocated in second.
+
+By default, compare prints out result message. This message displays
+information that both images are same or position of first different byte.
+In addition, result message can report different image size in case Strict
+mode is used.
+In case you want to suppress any non-error output, you can use Quiet mode by
+specifying @var{-q} option.
+
@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
2012-12-06 12:24 ` [Qemu-devel] [PATCH v6] " Miroslav Rezanina
@ 2012-12-11 9:16 ` Stefan Hajnoczi
2012-12-11 12:27 ` Kevin Wolf
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2012-12-11 9:16 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel
On Thu, Dec 06, 2012 at 07:24:04AM -0500, Miroslav Rezanina wrote:
> This is second version of patch adding compare subcommand that
> compares two images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has
> to be zeroed/unallocated to compare rest
> - qemu-img returns:
> - 0 if images are identical
> - 1 if images differ
> - 2 on error
>
> v6:
> - added handling -?, -h options for compare subcommand
>
> v5 (only minor changes):
> - removed redundant comment
> - removed dead code (goto after help())
> - set final total_sectors on first assignment
>
> v4:
> - Fixed various typos
> - Added functions for empty sector check and sector-to-bytes offset
> conversion
> - Fixed command-line parameters processing
>
> v3:
> - options -f/-F are orthogonal
> - documentation updated to new syntax and behavior
> - used byte offset instead of sector number for output
>
> v2:
> - changed option for second image format to -F
> - changed handling of -f and -F [1]
> - added strict mode (-s)
> - added quiet mode (-q)
> - improved output messages [2]
> - rename variables for larger image handling
> - added man page content
>
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
2012-12-06 12:24 ` [Qemu-devel] [PATCH v6] " Miroslav Rezanina
2012-12-11 9:16 ` Stefan Hajnoczi
@ 2012-12-11 12:27 ` Kevin Wolf
2012-12-11 13:09 ` Miroslav Rezanina
1 sibling, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2012-12-11 12:27 UTC (permalink / raw)
To: Miroslav Rezanina; +Cc: Paolo Bonzini, qemu-devel, Stefan Hajnoczi
Am 06.12.2012 13:24, schrieb Miroslav Rezanina:
> This is second version of patch adding compare subcommand that
> compares two images. Compare has following criteria:
> - only data part is compared
> - unallocated sectors are not read
> - in case of different image size, exceeding part of bigger disk has
> to be zeroed/unallocated to compare rest
> - qemu-img returns:
> - 0 if images are identical
> - 1 if images differ
> - 2 on error
>
> v6:
> - added handling -?, -h options for compare subcommand
>
> v5 (only minor changes):
> - removed redundant comment
> - removed dead code (goto after help())
> - set final total_sectors on first assignment
>
> v4:
> - Fixed various typos
> - Added functions for empty sector check and sector-to-bytes offset
> conversion
> - Fixed command-line parameters processing
>
> v3:
> - options -f/-F are orthogonal
> - documentation updated to new syntax and behavior
> - used byte offset instead of sector number for output
>
> v2:
> - changed option for second image format to -F
> - changed handling of -f and -F [1]
> - added strict mode (-s)
> - added quiet mode (-q)
> - improved output messages [2]
> - rename variables for larger image handling
> - added man page content
>
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
Please move the changelog below a "---" line so that git am ignores it
for the final commit message.
> diff --git a/block.c b/block.c
> index 854ebd6..fdc8c45 100644
> --- a/block.c
> +++ b/block.c
> @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>
> typedef struct BdrvCoIsAllocatedData {
> BlockDriverState *bs;
> + BlockDriverState *base;
> int64_t sector_num;
> int nb_sectors;
> int *pnum;
> @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
> return 0;
> }
>
> +/* Coroutine wrapper for bdrv_is_allocated_above() */
> +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
> +{
> + BdrvCoIsAllocatedData *data = opaque;
> + BlockDriverState *top = data->bs;
> + BlockDriverState *base = data->base;
> +
> + data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
> + data->nb_sectors, data->pnum);
> + data->done = true;
> +}
> +
> +/*
> + * Synchronous wrapper around bdrv_co_is_allocated_above().
> + *
> + * See bdrv_co_is_allocated_above() for details.
> + */
> +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> + int64_t sector_num, int nb_sectors, int *pnum)
> +{
> + Coroutine *co;
> + BdrvCoIsAllocatedData data = {
> + .bs = top,
> + .base = base,
> + .sector_num = sector_num,
> + .nb_sectors = nb_sectors,
> + .pnum = pnum,
> + .done = false,
> + };
> +
> + co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
> + qemu_coroutine_enter(co, &data);
> + while (!data.done) {
> + qemu_aio_wait();
> + }
> + return data.ret;
> +}
> +
> BlockInfo *bdrv_query_info(BlockDriverState *bs)
> {
> BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/block.h b/block.h
> index 722c620..2cb8d71 100644
> --- a/block.h
> +++ b/block.h
> @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
> int bdrv_has_zero_init(BlockDriverState *bs);
> int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> int *pnum);
> +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> + int64_t sector_num, int nb_sectors, int *pnum);
>
> void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
> BlockdevOnError on_write_error);
This first part looks good. I think it could be a separate patch, however.
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index a181363..c076085 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -27,6 +27,12 @@ STEXI
> @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
> ETEXI
>
> +DEF("compare", img_compare,
> + "compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2")
> +STEXI
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2}
> +ETEXI
> +
> DEF("convert", img_convert,
> "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
> STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..09abb3a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,13 @@ static void help(void)
> " '-a' applies a snapshot (revert disk to saved state)\n"
> " '-c' creates a snapshot\n"
> " '-d' deletes a snapshot\n"
> - " '-l' lists all snapshots in the given image\n";
> + " '-l' lists all snapshots in the given image\n"
> + "\n"
> + "Parameters to compare subcommand:\n"
> + " '-f' First image format\n"
> + " '-F' Second image format\n"
> + " '-q' Quiet mode - do not print any output (except errors)\n"
> + " '-s' Strict mode - fail on different image size or sector allocation\n";
Let's put -q in the section for common options. It's currently only in
compare, but it definitely makes sense for other subcommands.
>
> printf("%s\nSupported formats:", help_msg);
> bdrv_iterate_format(format_print, NULL);
> @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>
> #define IO_BUF_SIZE (2 * 1024 * 1024)
>
> +static int64_t sectors_to_bytes(int64_t sectors)
> +{
> + return sectors << BDRV_SECTOR_BITS;
> +}
> +
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +static int64_t sectors_to_process(int64_t total, int64_t from)
According to the comment it would always return IO_BUF_SIZE, so I would
suggest to rephrase it.
> +{
> + int64_t rv = total - from;
> +
> + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> + return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> + }
> +
> + return rv;
> +}
What does 'rv' mean? Return value? If so, the common name at least in
block layer code is 'ret'.
> +
> +/*
> + * Check if passed sectors are filled with 0.
> + *
> + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
> + * data and negative value on error.
> + */
> +static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
> + const char *filename, uint8_t *buffer, int quiet)
Shouldn't pass the file name here. Also 'bool quiet'.
A function name is_zeroed() suggests that the result is a boolean, i.e.
0 means "not zeroed" and > 0 means "zeroed".
> +{
> + int pnum, rv = 0;
> + rv = bdrv_read(bs, sect_num, buffer, sect_count);
> + if (rv < 0) {
> + error_report("Error while reading offset %" PRId64 " of %s: %s",
> + sectors_to_bytes(sect_num), filename, strerror(-rv));
If you want to include the file name, you can use the loc_*() functions.
> + return rv;
> + }
> + rv = is_allocated_sectors(buffer, sect_count, &pnum);
> + if (rv || pnum != sect_count) {
> + if (!quiet) {
> + printf("Content mismatch at offset %" PRId64 "!\n",
> + sectors_to_bytes(rv ? sect_num : sect_num + pnum));
Sure that this error message is at the right layer? The function header
comment doesn't say anything about the situation in which the function
must be used.
> + }
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occurred
> + */
> +static int img_compare(int argc, char **argv)
> +{
> + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
> + BlockDriverState *bs1, *bs2;
> + int64_t total_sectors1, total_sectors2;
> + uint8_t *buf1 = NULL, *buf2 = NULL;
> + int pnum1, pnum2;
> + int allocated1, allocated2;
> + int flags = BDRV_O_FLAGS;
This is never changed. You can use the constant directly.
> + int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
> + int progress = 0, quiet = 0, strict = 0;
bool
> + int64_t total_sectors;
> + int64_t sector_num = 0;
> + int64_t nb_sectors;
> + int c, rv, pnum;
> + uint64_t bs_sectors;
> + uint64_t progress_base;
> +
> +
> + for (;;) {
> + c = getopt(argc, argv, "hpf:F:sq");
> + if (c == -1) {
> + break;
> + }
> + switch (c) {
> + case '?':
> + case 'h':
> + help();
> + break;
> + case 'f':
> + fmt1 = optarg;
> + break;
> + case 'F':
> + fmt2 = optarg;
> + break;
> + case 'p':
> + progress = 1;
> + break;
> + case 'q':
> + quiet = 1;
> + break;
> + case 's':
> + strict = 1;
> + break;
> + }
> + }
> +
> + /* Progress is not shown in Quiet mode */
> + if (quiet) {
> + progress = 0;
> + }
> +
> + if (optind > argc - 2) {
> + help();
> + }
> + filename1 = argv[optind++];
> + filename2 = argv[optind++];
> +
> + /* Initialize before goto out */
> + qemu_progress_init(progress, 2.0);
> +
> + bs1 = bdrv_new_open(filename1, fmt1, flags, true);
> + if (!bs1) {
> + error_report("Can't open file %s", filename1);
> + ret = 2;
> + goto out3;
> + }
> +
> + bs2 = bdrv_new_open(filename2, fmt2, flags, true);
> + if (!bs2) {
> + error_report("Can't open file %s", filename2);
> + ret = 2;
> + goto out2;
> + }
> +
> + buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> + buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> + bdrv_get_geometry(bs1, &bs_sectors);
> + total_sectors1 = bs_sectors;
> + bdrv_get_geometry(bs2, &bs_sectors);
> + total_sectors2 = bs_sectors;
> + total_sectors = MIN(total_sectors1, total_sectors2);
> + progress_base = total_sectors;
> +
> + qemu_progress_print(0, 100);
> +
> + if (total_sectors1 != total_sectors2) {
> + BlockDriverState *bsover;
> + int64_t lo_total_sectors, lo_sector_num;
> + const char *filename_over;
> +
> + if (strict) {
> + ret = 1;
> + if (!quiet) {
> + printf("Strict mode: Image size mismatch!\n");
> + }
> + goto out;
> + } else {
> + error_report("Image size mismatch!");
> + }
> +
> + if (total_sectors1 > total_sectors2) {
> + lo_total_sectors = total_sectors1;
> + lo_sector_num = total_sectors2;
> + bsover = bs1;
> + filename_over = filename1;
> + } else {
> + lo_total_sectors = total_sectors2;
> + lo_sector_num = total_sectors1;
> + bsover = bs2;
> + filename_over = filename2;
> + }
> +
> + progress_base = lo_total_sectors;
> +
> + for (;;) {
> + nb_sectors = sectors_to_process(lo_total_sectors, lo_sector_num);
> + if (nb_sectors <= 0) {
> + break;
> + }
> + rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num,
> + nb_sectors, &pnum);
> + nb_sectors = pnum;
> + if (rv) {
> + rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
> + filename_over, buf1, quiet);
> + if (rv) {
> + if (rv < 0) {
> + ret = 2;
> + }
> + goto out;
> + }
> + }
> + lo_sector_num += nb_sectors;
> + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
> + }
> + }
Hm, nb_sectors is the number of sectors processed in one loop iteration,
isn't it? How can you calculate a meaningful progress from it then? And
even if it did work, wouldn't the progress bar jump backwards when it
starts comparing the part that exists in both images?
And didn't we decide that qemu-img compare should always return the
lowest offset at which the images differ?
> +
> +
> + for (;;) {
> + nb_sectors = sectors_to_process(total_sectors, sector_num);
> + if (nb_sectors <= 0) {
> + break;
> + }
> + allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
> + &pnum1);
> + allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
> + &pnum2);
> + if (pnum1 < pnum2) {
> + nb_sectors = pnum1;
> + } else {
> + nb_sectors = pnum2;
> + }
nb_sectors = MIN(pnum1, pnum2);
> @@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
>
> Commit the changes recorded in @var{filename} in its base image.
>
> +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
> +
> +Check if two images contains same content. You can compare images with
s/contains/contain the/
Or maybe even "have" the same content. Containing content is kind of
redundant.
> +different format or settings.
> +
> +Format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
s/Format/The format/
> +
> +By default, compare evaluates as identical images with different size where
> +bigger image contains only unallocated and/or zeroed sectors in area above
> +second image size.
Hm, let me try and rephrase this one...
"By default, images with different size are considered identical if the
larger image contains only unallocated and/or zeroed sectors in the area
after the end of the other image."
> In addition, if any sector is not allocated in one image
> +and contains only zero bytes in second, it is evaluated as equal.
s/in second/in the second one/
> You can use
> +Strict mode by specifying @var{-s} option. When compare runs in Strict mode,
_the_ -s option
> +it fails in case image size differs or sector is allocated in one image and
_a_ sector
> +is not allocated in second.
the second one.
> +
> +By default, compare prints out result message. This message displays
_a_ result message
> +information that both images are same or position of first different byte.
_the_ position of _the_ first different byte
> +In addition, result message can report different image size in case Strict
> +mode is used.
_the_ result message can report an image size difference
> +In case you want to suppress any non-error output, you can use Quiet mode by
> +specifying @var{-q} option.
_the_ -q option
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
2012-12-11 12:27 ` Kevin Wolf
@ 2012-12-11 13:09 ` Miroslav Rezanina
0 siblings, 0 replies; 24+ messages in thread
From: Miroslav Rezanina @ 2012-12-11 13:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Paolo Bonzini, qemu-devel, Stefan Hajnoczi
Hi Kevin,
thanks for review, comments inline.
----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent: Tuesday, December 11, 2012 1:27:45 PM
> Subject: Re: [PATCH v6] Add compare subcommand for qemu-img
>
> Am 06.12.2012 13:24, schrieb Miroslav Rezanina:
> > This is second version of patch adding compare subcommand that
> > compares two images. Compare has following criteria:
> > - only data part is compared
> > - unallocated sectors are not read
> > - in case of different image size, exceeding part of bigger disk
> > has
> > to be zeroed/unallocated to compare rest
> > - qemu-img returns:
> > - 0 if images are identical
> > - 1 if images differ
> > - 2 on error
> >
> > v6:
> > - added handling -?, -h options for compare subcommand
> >
> > v5 (only minor changes):
> > - removed redundant comment
> > - removed dead code (goto after help())
> > - set final total_sectors on first assignment
> >
> > v4:
> > - Fixed various typos
> > - Added functions for empty sector check and sector-to-bytes
> > offset
> > conversion
> > - Fixed command-line parameters processing
> >
> > v3:
> > - options -f/-F are orthogonal
> > - documentation updated to new syntax and behavior
> > - used byte offset instead of sector number for output
> >
> > v2:
> > - changed option for second image format to -F
> > - changed handling of -f and -F [1]
> > - added strict mode (-s)
> > - added quiet mode (-q)
> > - improved output messages [2]
> > - rename variables for larger image handling
> > - added man page content
> >
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>
> Please move the changelog below a "---" line so that git am ignores
> it
> for the final commit message.
>
Ok, next version probably be as multipart so this will go into the header mail.
> > diff --git a/block.c b/block.c
> > index 854ebd6..fdc8c45 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
> >
> > typedef struct BdrvCoIsAllocatedData {
> > BlockDriverState *bs;
> > + BlockDriverState *base;
> > int64_t sector_num;
> > int nb_sectors;
> > int *pnum;
> > @@ -2828,6 +2829,44 @@ int coroutine_fn
> > bdrv_co_is_allocated_above(BlockDriverState *top,
> > return 0;
> > }
> >
> > +/* Coroutine wrapper for bdrv_is_allocated_above() */
> > +static void coroutine_fn bdrv_is_allocated_above_co_entry(void
> > *opaque)
> > +{
> > + BdrvCoIsAllocatedData *data = opaque;
> > + BlockDriverState *top = data->bs;
> > + BlockDriverState *base = data->base;
> > +
> > + data->ret = bdrv_co_is_allocated_above(top, base,
> > data->sector_num,
> > + data->nb_sectors,
> > data->pnum);
> > + data->done = true;
> > +}
> > +
> > +/*
> > + * Synchronous wrapper around bdrv_co_is_allocated_above().
> > + *
> > + * See bdrv_co_is_allocated_above() for details.
> > + */
> > +int bdrv_is_allocated_above(BlockDriverState *top,
> > BlockDriverState *base,
> > + int64_t sector_num, int nb_sectors, int
> > *pnum)
> > +{
> > + Coroutine *co;
> > + BdrvCoIsAllocatedData data = {
> > + .bs = top,
> > + .base = base,
> > + .sector_num = sector_num,
> > + .nb_sectors = nb_sectors,
> > + .pnum = pnum,
> > + .done = false,
> > + };
> > +
> > + co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
> > + qemu_coroutine_enter(co, &data);
> > + while (!data.done) {
> > + qemu_aio_wait();
> > + }
> > + return data.ret;
> > +}
> > +
> > BlockInfo *bdrv_query_info(BlockDriverState *bs)
> > {
> > BlockInfo *info = g_malloc0(sizeof(*info));
> > diff --git a/block.h b/block.h
> > index 722c620..2cb8d71 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs,
> > int64_t sector_num, int nb_sectors);
> > int bdrv_has_zero_init(BlockDriverState *bs);
> > int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
> > int nb_sectors,
> > int *pnum);
> > +int bdrv_is_allocated_above(BlockDriverState *top,
> > BlockDriverState *base,
> > + int64_t sector_num, int nb_sectors,
> > int *pnum);
> >
> > void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError
> > on_read_error,
> > BlockdevOnError on_write_error);
>
> This first part looks good. I think it could be a separate patch,
> however.
>
Agree with this, was lazy to split.
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index a181363..c076085 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -27,6 +27,12 @@ STEXI
> > @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
> > ETEXI
> >
> > +DEF("compare", img_compare,
> > + "compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1
> > filename2")
> > +STEXI
> > +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s]
> > @var{filename1} @var{filename2}
> > +ETEXI
> > +
> > DEF("convert", img_convert,
> > "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o
> > options] [-s snapshot_name] [-S sparse_size] filename
> > [filename2 [...]] output_filename")
> > STEXI
> > diff --git a/qemu-img.c b/qemu-img.c
> > index e29e01b..09abb3a 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -101,7 +101,13 @@ static void help(void)
> > " '-a' applies a snapshot (revert disk to saved
> > state)\n"
> > " '-c' creates a snapshot\n"
> > " '-d' deletes a snapshot\n"
> > - " '-l' lists all snapshots in the given image\n";
> > + " '-l' lists all snapshots in the given image\n"
> > + "\n"
> > + "Parameters to compare subcommand:\n"
> > + " '-f' First image format\n"
> > + " '-F' Second image format\n"
> > + " '-q' Quiet mode - do not print any output (except
> > errors)\n"
> > + " '-s' Strict mode - fail on different image size or
> > sector allocation\n";
>
> Let's put -q in the section for common options. It's currently only
> in
> compare, but it definitely makes sense for other subcommands.
>
Ok, I can take care of this for other commands in next version (as one patch of the serie).
> >
> > printf("%s\nSupported formats:", help_msg);
> > bdrv_iterate_format(format_print, NULL);
> > @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t
> > *buf1, const uint8_t *buf2, int n,
> >
> > #define IO_BUF_SIZE (2 * 1024 * 1024)
> >
> > +static int64_t sectors_to_bytes(int64_t sectors)
> > +{
> > + return sectors << BDRV_SECTOR_BITS;
> > +}
> > +
> > +/*
> > + * Get number of sectors that can be stored in IO buffer.
> > + */
> > +static int64_t sectors_to_process(int64_t total, int64_t from)
>
> According to the comment it would always return IO_BUF_SIZE, so I
> would
> suggest to rephrase it.
>
Right, can is not correctly used here.
> > +{
> > + int64_t rv = total - from;
> > +
> > + if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> > + return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> > + }
> > +
> > + return rv;
> > +}
>
> What does 'rv' mean? Return value? If so, the common name at least in
> block layer code is 'ret'.
>
My fault, used to rv, fix it to ret in next version.
> > +
> > +/*
> > + * Check if passed sectors are filled with 0.
> > + *
> > + * Returns 0 in case sectors are filled with 0, 1 if sectors
> > contain non-zero
> > + * data and negative value on error.
> > + */
> > +static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int
> > sect_count,
> > + const char *filename, uint8_t *buffer, int quiet)
>
> Shouldn't pass the file name here. Also 'bool quiet'.
>
> A function name is_zeroed() suggests that the result is a boolean,
> i.e.
> 0 means "not zeroed" and > 0 means "zeroed".
>
Yeah, the function name is problem here. In fact it's purpose is to compare
passed block of data with block of zero bytes. It's common part for checking
bytes in bigger image and for case one image has block allocated and second hasn't.
That's the reason quiet and filename are passed and error handling is done in the
function - to minimaze duplicate code. So the question is, if I should rename the
function or make it more general.
> > +{
> > + int pnum, rv = 0;
> > + rv = bdrv_read(bs, sect_num, buffer, sect_count);
> > + if (rv < 0) {
> > + error_report("Error while reading offset %" PRId64 " of
> > %s: %s",
> > + sectors_to_bytes(sect_num), filename,
> > strerror(-rv));
>
> If you want to include the file name, you can use the loc_*()
> functions.
>
> > + return rv;
> > + }
> > + rv = is_allocated_sectors(buffer, sect_count, &pnum);
> > + if (rv || pnum != sect_count) {
> > + if (!quiet) {
> > + printf("Content mismatch at offset %" PRId64 "!\n",
> > + sectors_to_bytes(rv ? sect_num : sect_num +
> > pnum));
>
> Sure that this error message is at the right layer? The function
> header
> comment doesn't say anything about the situation in which the
> function
> must be used.
>
> > + }
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Compares two images. Exit codes:
> > + *
> > + * 0 - Images are identical
> > + * 1 - Images differ
> > + * 2 - Error occurred
> > + */
> > +static int img_compare(int argc, char **argv)
> > +{
> > + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
> > + BlockDriverState *bs1, *bs2;
> > + int64_t total_sectors1, total_sectors2;
> > + uint8_t *buf1 = NULL, *buf2 = NULL;
> > + int pnum1, pnum2;
> > + int allocated1, allocated2;
> > + int flags = BDRV_O_FLAGS;
>
> This is never changed. You can use the constant directly.
>
> > + int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error
> > */
> > + int progress = 0, quiet = 0, strict = 0;
>
> bool
>
> > + int64_t total_sectors;
> > + int64_t sector_num = 0;
> > + int64_t nb_sectors;
> > + int c, rv, pnum;
> > + uint64_t bs_sectors;
> > + uint64_t progress_base;
> > +
> > +
> > + for (;;) {
> > + c = getopt(argc, argv, "hpf:F:sq");
> > + if (c == -1) {
> > + break;
> > + }
> > + switch (c) {
> > + case '?':
> > + case 'h':
> > + help();
> > + break;
> > + case 'f':
> > + fmt1 = optarg;
> > + break;
> > + case 'F':
> > + fmt2 = optarg;
> > + break;
> > + case 'p':
> > + progress = 1;
> > + break;
> > + case 'q':
> > + quiet = 1;
> > + break;
> > + case 's':
> > + strict = 1;
> > + break;
> > + }
> > + }
> > +
> > + /* Progress is not shown in Quiet mode */
> > + if (quiet) {
> > + progress = 0;
> > + }
> > +
> > + if (optind > argc - 2) {
> > + help();
> > + }
> > + filename1 = argv[optind++];
> > + filename2 = argv[optind++];
> > +
> > + /* Initialize before goto out */
> > + qemu_progress_init(progress, 2.0);
> > +
> > + bs1 = bdrv_new_open(filename1, fmt1, flags, true);
> > + if (!bs1) {
> > + error_report("Can't open file %s", filename1);
> > + ret = 2;
> > + goto out3;
> > + }
> > +
> > + bs2 = bdrv_new_open(filename2, fmt2, flags, true);
> > + if (!bs2) {
> > + error_report("Can't open file %s", filename2);
> > + ret = 2;
> > + goto out2;
> > + }
> > +
> > + buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> > + buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> > + bdrv_get_geometry(bs1, &bs_sectors);
> > + total_sectors1 = bs_sectors;
> > + bdrv_get_geometry(bs2, &bs_sectors);
> > + total_sectors2 = bs_sectors;
> > + total_sectors = MIN(total_sectors1, total_sectors2);
> > + progress_base = total_sectors;
> > +
> > + qemu_progress_print(0, 100);
> > +
> > + if (total_sectors1 != total_sectors2) {
> > + BlockDriverState *bsover;
> > + int64_t lo_total_sectors, lo_sector_num;
> > + const char *filename_over;
> > +
> > + if (strict) {
> > + ret = 1;
> > + if (!quiet) {
> > + printf("Strict mode: Image size mismatch!\n");
> > + }
> > + goto out;
> > + } else {
> > + error_report("Image size mismatch!");
> > + }
> > +
> > + if (total_sectors1 > total_sectors2) {
> > + lo_total_sectors = total_sectors1;
> > + lo_sector_num = total_sectors2;
> > + bsover = bs1;
> > + filename_over = filename1;
> > + } else {
> > + lo_total_sectors = total_sectors2;
> > + lo_sector_num = total_sectors1;
> > + bsover = bs2;
> > + filename_over = filename2;
> > + }
> > +
> > + progress_base = lo_total_sectors;
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(lo_total_sectors,
> > lo_sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + rv = bdrv_is_allocated_above(bsover, NULL,
> > lo_sector_num,
> > + nb_sectors, &pnum);
> > + nb_sectors = pnum;
> > + if (rv) {
> > + rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
> > + filename_over, buf1, quiet);
> > + if (rv) {
> > + if (rv < 0) {
> > + ret = 2;
> > + }
> > + goto out;
> > + }
> > + }
> > + lo_sector_num += nb_sectors;
> > + qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> > + }
> > + }
>
> Hm, nb_sectors is the number of sectors processed in one loop
> iteration,
> isn't it? How can you calculate a meaningful progress from it then?
> And
> even if it did work, wouldn't the progress bar jump backwards when it
> starts comparing the part that exists in both images?
>
qemu_progress_print is quite a strange function and this is the correct usage even if
it looks suspicious. First argument is delta since last call not the decimal part.
> And didn't we decide that qemu-img compare should always return the
> lowest offset at which the images differ?
No we did not have any final decision yet. There were just two approaches and patch uses the one
I prefer (as the opposite approach was only suggested not requested).
My expectation is that compare subcommand returns (as return value) if the images are same or not.
Printed info where the images differ is not so important and is suppressed in quiet mode.
As compare should minimize the time used for compare, it checks non-shared portion of disk first
as it's faster and probability of difference is bigger.
>
> > +
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(total_sectors,
> > sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + allocated1 = bdrv_is_allocated_above(bs1, NULL,
> > sector_num, nb_sectors,
> > + &pnum1);
> > + allocated2 = bdrv_is_allocated_above(bs2, NULL,
> > sector_num, nb_sectors,
> > + &pnum2);
> > + if (pnum1 < pnum2) {
> > + nb_sectors = pnum1;
> > + } else {
> > + nb_sectors = pnum2;
> > + }
>
> nb_sectors = MIN(pnum1, pnum2);
>
> > @@ -114,6 +128,28 @@ it doesn't need to be specified separately in
> > this case.
> >
> > Commit the changes recorded in @var{filename} in its base image.
> >
> > +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q]
> > @var{filename1} @var{filename2}
> > +
> > +Check if two images contains same content. You can compare images
> > with
>
> s/contains/contain the/
>
> Or maybe even "have" the same content. Containing content is kind of
> redundant.
>
> > +different format or settings.
> > +
> > +Format is probed unless you specify it by @var{-f} (used for
> > @var{filename1}) and/or @var{-F} (used for @var{filename2})
> > option.
>
> s/Format/The format/
>
> > +
> > +By default, compare evaluates as identical images with different
> > size where
> > +bigger image contains only unallocated and/or zeroed sectors in
> > area above
> > +second image size.
>
> Hm, let me try and rephrase this one...
>
> "By default, images with different size are considered identical if
> the
> larger image contains only unallocated and/or zeroed sectors in the
> area
> after the end of the other image."
>
> > In addition, if any sector is not allocated in one image
> > +and contains only zero bytes in second, it is evaluated as equal.
>
> s/in second/in the second one/
>
> > You can use
> > +Strict mode by specifying @var{-s} option. When compare runs in
> > Strict mode,
>
> _the_ -s option
>
> > +it fails in case image size differs or sector is allocated in one
> > image and
>
> _a_ sector
>
> > +is not allocated in second.
>
> the second one.
>
> > +
> > +By default, compare prints out result message. This message
> > displays
>
> _a_ result message
>
> > +information that both images are same or position of first
> > different byte.
>
> _the_ position of _the_ first different byte
>
> > +In addition, result message can report different image size in
> > case Strict
> > +mode is used.
>
> _the_ result message can report an image size difference
>
> > +In case you want to suppress any non-error output, you can use
> > Quiet mode by
> > +specifying @var{-q} option.
>
> _the_ -q option
>
> Kevin
>
^ permalink raw reply [flat|nested] 24+ messages in thread