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

[Paolo Bonzini <pbonzini@redhat.com>]

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