Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img

[Kevin Wolf]

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