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

[Stefan Hajnoczi]

On Wed, Nov 21, 2012 at 04:50:14AM -0500, Miroslav Rezanina wrote:
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..6294b11 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,12 @@ 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"

Please add "\n" to separate like the other parameter docs for
subcommands.

> +           "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);
> @@ -657,6 +662,277 @@ static int compare_sectors(const uint8_t *buf1, const 
> uint8_t *buf2, int n,
>  }
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
> +#define sector_to_bytes(sector) ((sector) << BDRV_SECTOR_BITS)

No macros, please.  Hiding the sector conversion isn't consistent anyway
since further down in your patch you explicitly use >> BDRV_SECTOR_BITS.

Either open code or define static inline functions so we have type
information.

> +
> +/*
> + * Get number of sectors that can be stored in IO buffer.
> + */
> +
> +static int64_t sectors_to_process(int64_t total, int64_t from)

Doc comments fit snuggly against the function definition, no newline
please.  QEMU code isn't very consistent in doc comment formatting in
general but it does not use a newline here.

> +    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 = (quiet == 0) ? 1 : 0;
> +            break;
> +        case 'q':
> +            quiet = 1;
> +            if (progress == 1) {
> +                progress = 0;
> +            }
> +            break;

It's cleaner to silence progress after all options have been parsed than
to duplicate the quiet/progress checking.

/* -q overrides -p */
if (quiet) {
    progress = 0;
}

> +        case 's':
> +            strict = 1;
> +            break;
> +        }

case 'h':
case '?':
    help();
    break;

> +    }
> +    if (optind >= argc) {

This subcommand takes two filenames, so check the number of arguments is
correct:

if (optind + 1 >= 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, 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);

Stray ':'?

> +        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;
> +    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(bsover, lo_sector_num, nb_sectors, &pnum);

We should error out if a backing image has non-zero data:

rv = bdrv_is_allocated_above(bsover, NULL, lo_sector_num, nb_sectors &pnum);

> +            nb_sectors = pnum;
> +            if (rv) {
> +                rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors);
> +                if (rv < 0) {
> +                    error_report("error while reading data offset %" PRId64
> +                                 " of %s: %s", 
> sector_to_bytes(lo_sector_num),
> +                                 filename_over, strerror(-rv));
> +                    ret = 2;
> +                    goto out;
> +                }
> +                rv = is_allocated_sectors(buf1, nb_sectors, &pnum);
> +                if (rv || pnum != nb_sectors) {
> +                    ret = 1;
> +                    if (!quiet) {
> +                        printf("Content mismatch - offset %" PRId64
> +                               " not available in both images!\n",
> +                               sector_to_bytes(
> +                                   rv ? lo_sector_num : lo_sector_num + 
> pnum));
> +                    }
> +                    goto out;
> +                }

This is duplicated from the code below.

Please split this function up into helper functions.  It will make some
of the temporary variables go away too.