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

[Stefan Hajnoczi]

On Tue, Nov 27, 2012 at 9:03 AM, Miroslav Rezanina <address@hidden> wrote:
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..9cc4365 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";
>
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -658,6 +664,288 @@ static int compare_sectors(const uint8_t *buf1, const 
> uint8_t *buf2, int n,
>
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>
> +/*
> + * Helper functionto display offset in bytes insted of sector number.
> + */
> +static int64_t sectors_to_bytes(int64_t sectors)

The purpose of this function is clear from its name and code.  There
are typos in the doc comment.  Please drop the doc comment, it's not
needed.

> +/*
> + * 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;
> +    int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */
> +    int progress = 0, quiet = 0, strict = 0;
> +    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:F:sq");
> +        if (c == -1) {
> +            break;
> +        }
> +        switch (c) {
> +        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();
> +        goto out3;

help() never returns, the goto can be dropped.

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

total_sectors = MIN(total_sectors1, total_sectors2);

This way you don't need to update with it later on.

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

Please move this into a separate function.  It allows you to get rid
of the temporary variables:

int check_all_zeroes(BlockDriverState *bs, int64_t sector_num, int64_t
nb_sectors);

if (total_sectors1 > total_sectors2) {
    rv = check_all_zeroes(bs1, total_sectors2, total_sectors2 - total_sectors1);
} else {
    rv = check_all_zeroes(bs2, total_sectors1, total_sectors1 - total_sectors2);
}