[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img |
Date: |
Tue, 20 Nov 2012 13:36:37 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0 |
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
- Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img,
Kevin Wolf <=