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

[Kevin Wolf]

Am 06.12.2012 13:24, 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
> 
> v6:
>  - added handling -?, -h options for compare subcommand
> 
> v5 (only minor changes):
>  - removed redundant comment
>  - removed dead code (goto after help())
>  - set final total_sectors on first assignment
> 
> v4:
>  - Fixed various typos
>  - Added functions for empty sector check and sector-to-bytes offset
>  conversion
>  - Fixed command-line parameters processing
> 
> v3:
>  - options -f/-F are orthogonal
>  - documentation updated to new syntax and behavior
>  - used byte offset instead of sector number for output
>  
> v2:
>  - changed option for second image format to -F
>  - changed handling 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
> 
> Signed-off-by: Miroslav Rezanina <address@hidden>

Please move the changelog below a "---" line so that git am ignores it
for the final commit message.

> diff --git a/block.c b/block.c
> index 854ebd6..fdc8c45 100644
> --- a/block.c
> +++ b/block.c
> @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>  
>  typedef struct BdrvCoIsAllocatedData {
>      BlockDriverState *bs;
> +    BlockDriverState *base;
>      int64_t sector_num;
>      int nb_sectors;
>      int *pnum;
> @@ -2828,6 +2829,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;
> +}
> +
>  BlockInfo *bdrv_query_info(BlockDriverState *bs)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/block.h b/block.h
> index 722c620..2cb8d71 100644
> --- a/block.h
> +++ b/block.h
> @@ -278,6 +278,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, BlockdevOnError on_read_error,
>                         BlockdevOnError on_write_error);

This first part looks good. I think it could be a separate patch, however.

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index a181363..c076085 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] [-F fmt] [-p] [-q] [-s] filename1 filename2")
> +STEXI
> address@hidden 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 e29e01b..09abb3a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,13 @@ 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"
> +           "\n"
> +           "Parameters to compare subcommand:\n"
> +           "  '-f' First image format\n"
> +           "  '-F' Second image format\n"
> +           "  '-q' Quiet mode - do not print any output (except errors)\n"
> +           "  '-s' Strict mode - fail on different image size or sector 
> allocation\n";

Let's put -q in the section for common options. It's currently only in
compare, but it definitely makes sense for other subcommands.

>  
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t *buf1, const 
> uint8_t *buf2, int n,
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>  
> +static int64_t sectors_to_bytes(int64_t sectors)
> +{
> +    return sectors << BDRV_SECTOR_BITS;
> +}
> +
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +static int64_t sectors_to_process(int64_t total, int64_t from)

According to the comment it would always return IO_BUF_SIZE, so I would
suggest to rephrase it.

> +{
> +    int64_t rv = total - from;
> +
> +    if (rv > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> +        return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> +    }
> +
> +    return rv;
> +}

What does 'rv' mean? Return value? If so, the common name at least in
block layer code is 'ret'.

> +
> +/*
> + * Check if passed sectors are filled with 0.
> + *
> + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
> + * data and negative value on error.
> + */
> +static int is_zeroed(BlockDriverState *bs, int64_t sect_num, int sect_count,
> +              const char *filename, uint8_t *buffer, int quiet)

Shouldn't pass the file name here. Also 'bool quiet'.

A function name is_zeroed() suggests that the result is a boolean, i.e.
0 means "not zeroed" and > 0 means "zeroed".

> +{
> +    int pnum, rv = 0;
> +    rv = bdrv_read(bs, sect_num, buffer, sect_count);
> +    if (rv < 0) {
> +        error_report("Error while reading offset %" PRId64 " of %s: %s",
> +                     sectors_to_bytes(sect_num), filename, strerror(-rv));

If you want to include the file name, you can use the loc_*() functions.

> +        return rv;
> +    }
> +    rv = is_allocated_sectors(buffer, sect_count, &pnum);
> +    if (rv || pnum != sect_count) {
> +        if (!quiet) {
> +            printf("Content mismatch at offset %" PRId64 "!\n",
> +                   sectors_to_bytes(rv ? sect_num : sect_num + pnum));

Sure that this error message is at the right layer? The function header
comment doesn't say anything about the situation in which the function
must be used.

> +        }
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Compares two images. Exit codes:
> + *
> + * 0 - Images are identical
> + * 1 - Images differ
> + * 2 - Error occurred
> + */
> +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;

This is never changed. You can use the constant directly.

> +    int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
> +    int progress = 0, quiet = 0, strict = 0;

bool

> +    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, "hpf:F:sq");
> +        if (c == -1) {
> +            break;
> +        }
> +        switch (c) {
> +        case '?':
> +        case 'h':
> +            help();
> +            break;
> +        case 'f':
> +            fmt1 = optarg;
> +            break;
> +        case 'F':
> +            fmt2 = optarg;
> +            break;
> +        case 'p':
> +            progress = 1;
> +            break;
> +        case 'q':
> +            quiet = 1;
> +            break;
> +        case 's':
> +            strict = 1;
> +            break;
> +        }
> +    }
> +
> +    /* Progress is not shown in Quiet mode */
> +    if (quiet) {
> +        progress = 0;
> +    }
> +
> +    if (optind > argc - 2) {
> +        help();
> +    }
> +    filename1 = argv[optind++];
> +    filename2 = argv[optind++];
> +
> +    /* Initialize before goto out */
> +    qemu_progress_init(progress, 2.0);
> +
> +    bs1 = bdrv_new_open(filename1, fmt1, flags, true);
> +    if (!bs1) {
> +        error_report("Can't open file %s", filename1);
> +        ret = 2;
> +        goto out3;
> +    }
> +
> +    bs2 = bdrv_new_open(filename2, fmt2, flags, true);
> +    if (!bs2) {
> +        error_report("Can't open file %s", filename2);
> +        ret = 2;
> +        goto out2;
> +    }
> +
> +    buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> +    buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> +    bdrv_get_geometry(bs1, &bs_sectors);
> +    total_sectors1 = bs_sectors;
> +    bdrv_get_geometry(bs2, &bs_sectors);
> +    total_sectors2 = bs_sectors;
> +    total_sectors = MIN(total_sectors1, total_sectors2);
> +    progress_base = total_sectors;
> +
> +    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!\n");
> +            }
> +            goto out;
> +        } else {
> +            error_report("Image size mismatch!");
> +        }
> +
> +        if (total_sectors1 > total_sectors2) {
> +            lo_total_sectors = total_sectors1;
> +            lo_sector_num = total_sectors2;
> +            bsover = bs1;
> +            filename_over = filename1;
> +        } else {
> +            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_above(bsover, NULL, lo_sector_num,
> +                                         nb_sectors, &pnum);
> +            nb_sectors = pnum;
> +            if (rv) {
> +                rv = is_zeroed(bsover, lo_sector_num, nb_sectors,
> +                               filename_over, buf1, quiet);
> +                if (rv) {
> +                    if (rv < 0) {
> +                        ret = 2;
> +                    }
> +                    goto out;
> +                }
> +            }
> +            lo_sector_num += nb_sectors;
> +            qemu_progress_print(((float) nb_sectors / progress_base)*100, 
> 100);
> +        }
> +    }

Hm, nb_sectors is the number of sectors processed in one loop iteration,
isn't it? How can you calculate a meaningful progress from it then? And
even if it did work, wouldn't the progress bar jump backwards when it
starts comparing the part that exists in both images?

And didn't we decide that qemu-img compare should always return the
lowest offset at which the images differ?

> +
> +
> +    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;
> +        }

nb_sectors = MIN(pnum1, pnum2);

> @@ -114,6 +128,28 @@ it doesn't need to be specified separately in this case.
>  
>  Commit the changes recorded in @var{filename} in its base image.
>  
> address@hidden compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] 
> @var{filename1} @var{filename2}
> +
> +Check if two images contains same content. You can compare images with

s/contains/contain the/

Or maybe even "have" the same content. Containing content is kind of
redundant.

> +different format or settings.
> +
> +Format is probed unless you specify it by @var{-f} (used for 
> @var{filename1}) and/or @var{-F} (used for @var{filename2}) option.

s/Format/The format/

> +
> +By default, compare evaluates as identical images with different size where
> +bigger image contains only unallocated and/or zeroed sectors in area above
> +second image size.

Hm, let me try and rephrase this one...

"By default, images with different size are considered identical if the
larger image contains only unallocated and/or zeroed sectors in the area
after the end of the other image."

> In addition, if any sector is not allocated in one image
> +and contains only zero bytes in second, it is evaluated as equal.

s/in second/in the second one/

> You can use
> +Strict mode by specifying @var{-s} option. When compare runs in Strict mode,

_the_ -s option

> +it fails in case image size differs or sector is allocated in one image and

_a_ sector

> +is not allocated in second.

the second one.

> +
> +By default, compare prints out result message. This message displays

_a_ result message

> +information that both images are same or position of first different byte.

_the_ position of _the_ first different byte

> +In addition, result message can report different image size in case Strict
> +mode is used.

_the_ result message can report an image size difference

> +In case you want to suppress any non-error output, you can use Quiet mode by
> +specifying @var{-q} option.

_the_ -q option

Kevin