From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tan4K-00058A-C4 for qemu-devel@nongnu.org; Tue, 20 Nov 2012 07:36:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tan4D-0008HK-TT for qemu-devel@nongnu.org; Tue, 20 Nov 2012 07:36:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:18544) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tan4D-0008Gr-Ld for qemu-devel@nongnu.org; Tue, 20 Nov 2012 07:36:41 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAKCaeCL006876 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 20 Nov 2012 07:36:40 -0500 Message-ID: <50AB7955.5040604@redhat.com> Date: Tue, 20 Nov 2012 13:36:37 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1736118218.6697488.1343814369561.JavaMail.root@redhat.com> <490157254.6767622.1343815423950.JavaMail.root@redhat.com> <20120803064520.GA10029@lws.brq.redhat.com> In-Reply-To: <20120803064520.GA10029@lws.brq.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Miroslav Rezanina Cc: pbonzini@redhat.com, qemu-devel@nongnu.org Am 03.08.2012 08:45, 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 > > v2: > - changed option for second image format to -F > - changed handlig 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 > > [1] Original patch handling was as following: > i) neither -f nor -F - both images probed for type > ii) -f only - both images use specified type > iii) -F only - first image probed, second image use specified type > iii) -f and -F - first image use -f type, second use -F type > > This patch change behavior in way that case ii) and iii) has same efect - we > use specified value for both images. > > [2] When we hit different sector we print its number out. > > Points to dicuss: > > i) Handling -f/-F options. > Currently we have three scenarios - no option > specified - probe all, one of options specified - use it for both, both option > specified - use each value for related image. This behavior is based on idea > that we can use format probing for all images or specify format for all images. > This preserve state when -f fmt specify input image format (compare is only > subcomand with more than one input image except convert that uses multiple > images without possibility to specify different format for each image). > > However, there's one more behavior to be considered - to use -f/-F for one > image only - when only one option is provided, only appropriate image use specified > format, second one is probed. Like Eric, I would prefer this alternative behaviour, so that one option is clearly related to only one image. > ii) How to handle images with different size. > If size of images is different and strict mode is not used, addditional size of > bigger image is checked to be zeroed/unallocated. This version do this check > before rest of image is compared. This is done to not compare whole image in > case that one of images is only expanded copy of other. > > Paolo Bonzini proposed to do this check after compare shared size of images to > go through image sequentially. I think the expected semantics is that the tool doesn't print the offset of just any difference, but of the first one. So I'd agree with Paolo here. > + 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!"); Missing \n? I also wonder if it would make sense to implement something like a qprintf(bool quiet, const char* fmt, ...) so that you don't have this verbose if (!quiet) around each message. Also, shouldn't this one be on stderr and be displayed even with -q? > + } > + goto out; > + } else if (!quiet) { > + printf("Warning: Image size mismatch!\n"); > + } > + > + 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); > + nb_sectors = pnum; > + if (rv) { > + rv = bdrv_read(bsover, lo_sector_num, buf1, nb_sectors); > + if (rv < 0) { > + error_report("error while reading sector %" PRId64 > + " of %s: %s", lo_sector_num, filename_over, > + strerror(-rv)); Please change the unit of all offsets from sectors to bytes, it's much friendlier if you don't have to know that qemu uses an arbitrary unit of 512 bytes internally. > + ret = 2; > + goto out; > + } > + rv = is_allocated_sectors(buf1, nb_sectors, &pnum); > + if (rv || pnum != nb_sectors) { > + ret = 1; > + if (!quiet) { > + printf("Content mismatch - Sector %" PRId64 > + " not available in both images!\n", > + rv ? lo_sector_num : lo_sector_num + pnum); Same here, plus stderr and display even with -q? (More instances follow, won't comment on each) > + } > + goto out; > + } > + } > + lo_sector_num += nb_sectors; > + qemu_progress_print(((float) nb_sectors / progress_base)*100, 100); > + } > + } > + > + > + 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; > + } > + > + 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) { > + ret = 1; > + if (!quiet) { > + printf("Content mismatch at sector %" PRId64 "!\n", > + rv ? sector_num : sector_num + pnum); Same here. Kevin