Re: [Qemu-devel] [PATCH 3/4] This patch adds new qemu-img subcommand that compares content of two disk images.

[Eric Blake]

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.
>  
> 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.

> +
> +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:
> +
> 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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature