[v2,RFC] Add compare subcommand for qemu-img

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(-)

--

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?

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