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).
> +/*
> + * 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.
> 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.
> +
> +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.
> +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?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
