qemu-devel
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]