From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWXK-0002wp-Ta for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:52:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwWXE-0003m7-SC for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:52:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54017) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWXE-0003lo-J4 for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:52:12 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q71AqBb9017596 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 1 Aug 2012 06:52:11 -0400 Message-ID: <50190A59.2050608@redhat.com> Date: Wed, 01 Aug 2012 12:52:09 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <579280363.6931076.1343817896059.JavaMail.root@redhat.com> In-Reply-To: <579280363.6931076.1343817896059.JavaMail.root@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH][RFC] Add compare subcommand for qemu-img List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Miroslav Rezanina Cc: qemu-devel@nongnu.org Il 01/08/2012 12:44, Miroslav Rezanina ha scritto: >> >> Also, perhaps we can add a "strict" mode (-S) that would fail if the >> images are of different size, and if a sector is allocated in one but >> unallocated in the other? >> >> And a "silent" or "quiet" mode (-s or -q, your choice) that prints >> nothing, too. >> > > Good idea with additional modes. I'm not sure if strict should fail on > allocated/unallocated - you can compare sparse/non-sparce when is this > highly probable. For that use case you probably know in advance that the size is the same, so you can use normal non-strict mode. Comparing allocated/unallocated on the other hand is helpful with unit tests. >> You can make total_sectors1/2 unsigned, and avoid the assignment. >> > This is using similar construct as img_convert I inspire in. I will > change this in v2. Oh, sorry then---consistency is better, please keep it as it is now. >>> > > + bdrv_get_geometry(bs2,&bs_sectors); >>> > > + total_sectors2 = bs_sectors; >>> > > + total_sectors = total_sectors1; >>> > > + progress_base = total_sectors; >> > >> > over_sectors = MAX(total_sectors1, total_sectors2); >> > total_sector = MIN(total_sectors1, total_sectors2); >> > over_num = over_sectors - total_sectors; >> > >>> > > + if (total_sectors1 != total_sectors2) { >> > >> > Using MAX/MIN as above, you can move this part to after you checked >> > the >> > common part of the image, so that images are read sequentially. >> > >> > You can still place the test here in strict mode. With the >> > assignments >> > given above, you can do the test simply like this: >> > >> > if (over_num > 0) { >> > } >> > > I want to have this check prior common part check - we do not have > to check rest of the image if we know there's something in this area > of disk. But if one of the two images is preallocated raw, or qcow2 with preallocated metadata, or your filesystem does not support is_allocated on raw images, then you're still reading all of the image just to check against zeros. Admittedly there is no single correct solution. Hopefully other reviewers will break the tie. Paolo