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

[Paolo Bonzini]

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 <address@hidden>
> 
> 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
> address@hidden 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.

> +        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.

> +    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) {
   }

> +        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.

> +                   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.

> +                  ret = 1;
> +                  printf("Images are different - content mismatch!\n");

Please print the sector number here.

> +                  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.

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