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

[Miroslav Rezanina]

Hi Kevin,
thanks for review, comments inline.

----- Original Message -----
> From: "Kevin Wolf" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden, "Paolo Bonzini" <address@hidden>, "Stefan Hajnoczi" 
> <address@hidden>
> Sent: Tuesday, December 11, 2012 1:27:45 PM
> Subject: Re: [PATCH v6] Add compare subcommand for qemu-img
> 
> 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.
> 
Ok, next version probably be as multipart so this will go into the header mail.

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

Agree with this, was lazy to split.

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

Ok, I can take care of this for other commands in next version (as one patch of 
the serie).

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

Right, can is not correctly used here.

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

My fault, used to rv, fix it to ret in next version.

> > +
> > +/*
> > + * 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".
> 

Yeah, the function name is problem here. In fact it's purpose is to compare
passed block of data with block of zero bytes. It's common part for checking 
bytes in bigger image and for case one image has block allocated and second 
hasn't.
That's the reason quiet and filename are passed and error handling is done in 
the
function - to minimaze duplicate code. So the question is, if I should rename 
the
function or make it more general.

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

qemu_progress_print is quite a strange function and this is the correct usage 
even if
it looks suspicious. First argument is delta since last call not the decimal 
part.

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

No we did not have any final decision yet. There were just two approaches and 
patch uses the one
I prefer (as the opposite approach was only suggested not requested).

My expectation is that compare subcommand returns (as return value) if the 
images are same or not. 
Printed info where the images differ is not so important and is suppressed in 
quiet mode. 
As compare should minimize the time used for compare, it checks non-shared 
portion of disk first 
as it's faster and probability of difference is bigger.

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