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

[Miroslav Rezanina <mrezanin@redhat.com>]

----- Original Message -----
> From: "Eric Blake" <eblake@redhat.com>
> To: mrezanin@redhat.com
> Cc: qemu-devel@nongnu.org, kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com
> Sent: Wednesday, December 19, 2012 7:12:49 PM
> Subject: Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand
> 
> 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.

You're right. I probably just see iff and change it to if without checking
the reason.

> 
> >   * 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?
> 

I think it's not. This should be save as block driver should not allow sectors to
cause 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?
> 
This is correct order. Function is_allocated_sector test if sectors contain any non-zero byte. We know that sector is "physically" allocated in the image, we test if it contains any data.
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

Miroslav Rezanina