----- Original Message ----- > From: "Eric Blake" <ebl...@redhat.com> > To: "Miroslav Rezanina" <mreza...@redhat.com> > Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, > stefa...@redhat.com > 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. > > > > +@item 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: > > + > > +@table @option > > + > > +@item -1 > > +Error on opening an image > > +@item -2 > > +Error on cheking a sector allorcation > > s/cheking/checking/ > s/allorcation/allocation/ > > > +@item -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