Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On 12/17/2012 06:39 AM, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> This patch adds new qemu-img subcommand that compare content of two disk

s/compare/compares/

> images.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
> @@ -587,7 +587,7 @@ static int img_commit(int argc, char **argv)
>  }
>  
>  /*
> - * Returns true iff the first sector pointed to by 'buf' contains at least
> + * Returns true if the first sector pointed to by 'buf' contains at least

Spurious change.  'iff' is correct here, for its mathematical meaning of
if-and-only-if.

>   * a non-NUL byte.
>   *
>   * 'pnum' is set to the number of sectors (including and immediately following
> @@ -688,6 +688,272 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
>  
> +static int64_t sectors_to_bytes(int64_t sectors)
> +{
> +    return sectors << BDRV_SECTOR_BITS;

Worth checking for overflow?

> +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num,
> +                               int sect_count, const char *filename,
> +                               uint8_t *buffer, bool quiet)
> +{
> +    int pnum, ret = 0;
> +    ret = bdrv_read(bs, sect_num, buffer, sect_count);
> +    if (ret < 0) {
> +        error_report("Error while reading offset %" PRId64 " of %s: %s",
> +                     sectors_to_bytes(sect_num), filename, strerror(-ret));
> +        return ret;
> +    }
> +    ret = is_allocated_sectors(buffer, sect_count, &pnum);

Is this logic backwards?  Isn't it wasteful to read a sector prior to
seeing if it was actually allocated?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]