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

[Miroslav Rezanina]

----- Original Message -----
> From: "Kevin Wolf" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden, address@hidden
> Sent: Tuesday, November 20, 2012 1:36:37 PM
> Subject: Re: [Qemu-devel] [PATCH v2][RFC] Add compare subcommand for qemu-img
> 
> 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.
>

Yeah, agree that one option affects one image only. This will be changed.

> > 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.

Question is -  what do expected. I would expect to not even bother with content
in case images differ in size - this is done on strict mode. In default mode
different size is ignored in case it is empty. This mean that "same size or
zero additional space" is condition preceeding condition "all sectors are same".
User should get "Images differ in size" message instead of "Sector XXX 
mismatch".

Different size can suggest wrong images are compared, but different sector could
leads to search what change this sector in one of the images? 
 
 
> 
> > +    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.

Yes, this is good idea.
> 
> Also, shouldn't this one be on stderr and be displayed even with -q?

I don't think so. It's not error, different images are one of possible results
we ask for - Output is: same/different (with reason in normal mode).


> 
> > +            }
> > +            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.

Ok, will be changed.

> 
> > +                    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)

See above.

> 
> > +                    }
> > +                    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.
> 

See above

> Kevin
> 

Thanks for comments,
Mirek