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

[Miroslav Rezanina]

----- Original Message -----
> From: "Paolo Bonzini" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden
> Sent: Wednesday, August 1, 2012 12:22:50 PM
> Subject: Re: [PATCH][RFC] Add compare subcommand for qemu-img
> 
> 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.
> 

Good idea with additional modes. I'm not sure if strict should fail on 
allocated/unallocated - you can compare sparse/non-sparce when is this
highly probable.

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

This is using similar construct as img_convert I inspire in. I will
change this in v2.

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

I want to have this check prior common part check - we do not have
to check rest of the image if we know there's something in this area
of disk.

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

Ok

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

We have to check this as we compare more than one sector. If the first
one will be same but any other will not we get 0 return value and expect
whole chunk to be same.

> 
> > +                  ret = 1;
> > +                  printf("Images are different - content
> > mismatch!\n");
> 
> Please print the sector number here.

Ok.

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

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

I will update patch as you commented and resend.

Mirek