Re: [Qemu-devel] [PATCH v4 13/23] qemu-img: Simplify logic in img_compare()

[John Snow]

On 09/13/2017 12:03 PM, Eric Blake wrote:
> As long as we are querying the status for a chunk smaller than
> the known image size, we are guaranteed that a successful return
> will have set pnum to a non-zero size (pnum is zero only for
> queries beyond the end of the file).  Use that to slightly
> simplify the calculation of the current chunk size being compared.
> Likewise, we don't have to shrink the amount of data operated on
> until we know we have to read the file, and therefore have to fit
> in the bounds of our buffer.  Also, note that 'total_sectors_over'
> is equivalent to 'progress_base'.
> 
> With these changes in place, sectors_to_process() is now dead code,
> and can be removed.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v3: new patch
> ---
>  qemu-img.c | 40 +++++++++++-----------------------------
>  1 file changed, 11 insertions(+), 29 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b91133b922..f8423e9b3f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1171,11 +1171,6 @@ static int64_t sectors_to_bytes(int64_t sectors)
>      return sectors << BDRV_SECTOR_BITS;
>  }
> 
> -static int64_t sectors_to_process(int64_t total, int64_t from)
> -{
> -    return MIN(total - from, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
> -}
> -
>  /*
>   * Check if passed sectors are empty (not allocated or contain only 0 bytes)
>   *
> @@ -1372,13 +1367,9 @@ static int img_compare(int argc, char **argv)
>          goto out;
>      }
> 
> -    for (;;) {
> +    while (sector_num < total_sectors) {
>          int64_t status1, status2;
> 
> -        nb_sectors = sectors_to_process(total_sectors, sector_num);
> -        if (nb_sectors <= 0) {
> -            break;
> -        }
>          status1 = bdrv_block_status_above(bs1, NULL,
>                                            sector_num * BDRV_SECTOR_SIZE,
>                                            (total_sectors1 - sector_num) *
> @@ -1402,14 +1393,9 @@ static int img_compare(int argc, char **argv)
>              goto out;
>          }
>          allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
> -        if (pnum1) {
> -            nb_sectors = MIN(nb_sectors,
> -                             DIV_ROUND_UP(pnum1, BDRV_SECTOR_SIZE));
> -        }
> -        if (pnum2) {
> -            nb_sectors = MIN(nb_sectors,
> -                             DIV_ROUND_UP(pnum2, BDRV_SECTOR_SIZE));
> -        }
> +
> +        assert(pnum1 && pnum2);
> +        nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);

In the apocalyptic future where non-sector sized returns are possible,
does this math make sense?

e.g. say the return is zeroes, but it's not aligned anymore, so we
assume we have an extra half a sector's worth of zeroes here.

> 
>          if (strict) {
>              if ((status1 & ~BDRV_BLOCK_OFFSET_MASK) !=
> @@ -1422,9 +1408,10 @@ static int img_compare(int argc, char **argv)
>              }
>          }
>          if ((status1 & BDRV_BLOCK_ZERO) && (status2 & BDRV_BLOCK_ZERO)) {
> -            nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);
> +            /* nothing to do */
>          } else if (allocated1 == allocated2) {
>              if (allocated1) {
> +                nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> 
> BDRV_SECTOR_BITS);
>                  ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
>                                  nb_sectors << BDRV_SECTOR_BITS);
>                  if (ret < 0) {
> @@ -1453,7 +1440,7 @@ static int img_compare(int argc, char **argv)
>                  }
>              }
>          } else {
> -
> +            nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
>              if (allocated1) {
>                  ret = check_empty_sectors(blk1, sector_num, nb_sectors,
>                                            filename1, buf1, quiet);
> @@ -1476,30 +1463,24 @@ static int img_compare(int argc, char **argv)
> 
>      if (total_sectors1 != total_sectors2) {
>          BlockBackend *blk_over;
> -        int64_t total_sectors_over;
>          const char *filename_over;
> 
>          qprintf(quiet, "Warning: Image size mismatch!\n");
>          if (total_sectors1 > total_sectors2) {
> -            total_sectors_over = total_sectors1;
>              blk_over = blk1;
>              filename_over = filename1;
>          } else {
> -            total_sectors_over = total_sectors2;
>              blk_over = blk2;
>              filename_over = filename2;
>          }
> 
> -        for (;;) {
> +        while (sector_num < progress_base) {
>              int64_t count;
> 
> -            nb_sectors = sectors_to_process(total_sectors_over, sector_num);
> -            if (nb_sectors <= 0) {
> -                break;
> -            }
>              ret = bdrv_is_allocated_above(blk_bs(blk_over), NULL,
>                                            sector_num * BDRV_SECTOR_SIZE,
> -                                          nb_sectors * BDRV_SECTOR_SIZE,
> +                                          (progress_base - sector_num) *
> +                                          BDRV_SECTOR_SIZE,
>                                            &count);
>              if (ret < 0) {
>                  ret = 3;
> @@ -1513,6 +1494,7 @@ static int img_compare(int argc, char **argv)
>              assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
>              nb_sectors = count >> BDRV_SECTOR_BITS;
>              if (ret) {
> +                nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> 
> BDRV_SECTOR_BITS);
>                  ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
>                                            filename_over, buf1, quiet);
>                  if (ret) {
> 

Rest looks right to me.