[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img
From: |
Miroslav Rezanina |
Subject: |
Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img |
Date: |
Wed, 1 Aug 2012 06:44:56 -0400 (EDT) |
----- Original Message -----
> From: "Paolo Bonzini" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden
> 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 <address@hidden>
> >
> > 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
> > address@hidden 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