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.
