Re: [PATCH v3 1/4] qemu-img: implement compare --stat

[Hanna Reitz]

On 28.10.21 12:24, Vladimir Sementsov-Ogievskiy wrote:
> With new option qemu-img compare will not stop at first mismatch, but
> instead calculate statistics: how many clusters with different data,
> how many clusters with equal data, how many clusters were unallocated
> but become data and so on.
>
> We compare images chunk by chunk. Chunk size depends on what
> block_status returns for both images. It may return less than cluster
> (remember about qcow2 subclusters), it may return more than cluster (if
> several consecutive clusters share same status). Finally images may
> have different cluster sizes. This all leads to ambiguity in how to
> finally compare the data.
>
> What we can say for sure is that, when we compare two qcow2 images with
> same cluster size, we should compare clusters with data separately.
> Otherwise, if we for example compare 10 consecutive clusters of data
> where only one byte differs we'll report 10 different clusters.
> Expected result in this case is 1 different cluster and 9 equal ones.
>
> So, to serve this case and just to have some defined rule let's do the
> following:
>
> 1. Select some block-size for compare procedure. In this commit it must
>     be specified by user, next commit will add some automatic logic and
>     make --block-size optional.
>
> 2. Go chunk-by-chunk using block_status as we do now with only one
>     differency:
>     If block_status() returns DATA region that intersects block-size
>     aligned boundary, crop this region at this boundary.
>
> This way it's still possible to compare less than cluster and report
> subcluster-level accuracy, but we newer compare more than one cluster
> of data.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/tools/qemu-img.rst |  18 +++-
>   qemu-img.c              | 210 +++++++++++++++++++++++++++++++++++++---
>   qemu-img-cmds.hx        |   4 +-
>   3 files changed, 216 insertions(+), 16 deletions(-)

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index f036a1d428..0cb7cebe91 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1465,7 +1602,7 @@ static int img_compare(int argc, char **argv)
>       }
>   
>       while (offset < total_size) {
> -        int status1, status2;
> +        block_end = QEMU_ALIGN_UP(offset + 1, block_size);

Without --stat, `block_size` is 0, and then this is a division by zero.

My compiler seems clever enough to skip this division if `stat == NULL`, 
but when I add a `printf("%li\n", block_size);` after this line and do a 
compare without --stat, qemu-img aborts.  I don’t think we should rely 
on the compiler optimization working here.

(Sorry I didn’t notice this in v2, I just noticed it because I was 
trying to find out whether `block_size` really needs to be a power of 
two as Eric proposed...  The good news is that I don’t think it needs to 
be a power of two (still might make sense to require it), but, well, the 
bad news is that I found this.)

Hanna