[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand
From: |
Miroslav Rezanina |
Subject: |
Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand |
Date: |
Thu, 20 Dec 2012 14:44:09 -0500 (EST) |
----- Original Message -----
> From: "Eric Blake" <address@hidden>
> To: address@hidden
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> 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, address@hidden wrote:
> > From: Miroslav Rezanina <address@hidden>
> >
> > This patch adds new qemu-img subcommand that compare content of two
> > disk
>
> s/compare/compares/
>
> > images.
> >
> > Signed-off-by: Miroslav Rezanina <address@hidden>
> > ---
> > @@ -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
Re: [Qemu-devel] [PATCH v7 1/4] block: Add synchronous wrapper for bdrv_co_is_allocated_above, Eric Blake, 2012/12/19