[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] This patch adds new qemu-img subcommand tha
From: |
Miroslav Rezanina |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] This patch adds new qemu-img subcommand that compares content of two disk images. |
Date: |
Mon, 11 Feb 2013 01:44:19 -0500 (EST) |
----- Original Message -----
> From: "Eric Blake" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> Sent: Friday, February 8, 2013 8:19:45 PM
> Subject: Re: [PATCH 3/4] This patch adds new qemu-img subcommand that
> compares content of two disk images.
>
> On 02/08/2013 01:33 AM, Miroslav Rezanina wrote:
> > This patch adds new qemu-img subcommand that compares content of
> > two disk
> > images.
> >
>
> > +static int64_t sectors_to_process(int64_t total, int64_t from)
> > +{
> > + return MIN((total - from), (IO_BUF_SIZE >> BDRV_SECTOR_BITS));
>
> Why the spurious ()? This can be written:
>
> return MIN(total - from, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
>
> unless the definition of MIN() is horribly busted (in which case, fix
> the broken macro, don't make callers suffer).
>
Just my habit - do not trust code you did not write (and not event the one you
wrote).
> > +/*
> > + * Compares two images. Exit codes:
> > + *
> > + * 0 - Images are identical
> > + * 1 - Images differ
> > + * <0 - Error occurred
> > + */
> > +static int img_compare(int argc, char **argv)
> > +{
> > + const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
> > + BlockDriverState *bs1, *bs2;
> > + int64_t total_sectors1, total_sectors2;
> > + uint8_t *buf1 = NULL, *buf2 = NULL;
> > + int pnum1, pnum2;
> > + int allocated1, allocated2;
> > + int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error
> > */
>
> This comment disagrees with the comment at the top of the function.
> I
> would just delete the comment here, instead of trying to keep them in
> sync.
>
Yeah, I miss this one. Thx for pointing it out.
> > Commit the changes recorded in @var{filename} in its base image.
> >
> > address@hidden compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q]
> > @var{filename1} @var{filename2}
>
> While this line is okay long...
>
> > +
> > +Check if two images have the same content. You can compare images
> > with
> > +different format or settings.
> > +
> > +The format is probed unless you specify it by @var{-f} (used for
> > @var{filename1}) and/or @var{-F} (used for @var{filename2})
> > option.
>
> ...this line should be wrapped.
>
You're right..
> > +
> > +Compare exits with @code{0} in case the images are equal and with
> > @code{1}
> > +in case the images differ. Negative exit code means an error
> > occured during
>
> s/occured/occurred/
>
> There's no such thing as a negative exit in shell. Rather, you
> should
> document that an exit code > 1 indicates an error during execution.
>
Oh, I was not aware of this fact. Thanks for explaining, have to be fixed.
> > +execution and standard error output should contain an error
> > message.
> > +Error exit codes are:
> > +
> > address@hidden @option
> > +
> > address@hidden -1
> > +Error on opening an image
> > address@hidden -2
> > +Error on cheking a sector allorcation
>
> s/cheking/checking/
> s/allorcation/allocation/
>
> > address@hidden -3
> > +Error on reading data
>
> Does it make sense to be this fine-grained in exit reporting? I'd be
> okay with blindly using exit status 2 for all failures. But if you
> really do want to call out this many different statuses, I'd list a
> single table for ALL possible exits, rather than prose about success
> and
> table for failure, as in:
>
> 0 - images are identical
> 1 - images differ
> 2 - error opening an image
> 3 - error checking sector allocation
> 4 - error reading from an image
>
> Isn't failure to determine sector allocation a subset of failure to
> read
> from an image?
>
This table is describing behavior prior the last change. I forget
to update it so the values are not correct. However, allocation
checking error is (or should be) on bdrv_is_allocated_above call
and reading error is on bdrv_read. (Yeah, it can be problem with
reading but this is hidden in bdrv).
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
--
Miroslav Rezanina
Software Engineer - Virtualization Team