From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWQK-0007h0-Dj for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:45:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwWQD-0000z0-HW for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:45:04 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:37341) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWQD-0000ys-2X for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:44:57 -0400 Date: Wed, 1 Aug 2012 06:44:56 -0400 (EDT) From: Miroslav Rezanina Message-ID: <579280363.6931076.1343817896059.JavaMail.root@redhat.com> In-Reply-To: <5019037A.8040601@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org ----- Original Message ----- > From: "Paolo Bonzini" > To: "Miroslav Rezanina" > 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 > > > > 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