[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand
From: |
Miroslav Rezanina |
Subject: |
Re: [Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand |
Date: |
Tue, 29 Jan 2013 07:04:41 -0500 (EST) |
See response inline, if no response that comment will be handled in next
version.
----- Original Message -----
> From: "Kevin Wolf" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden
> Sent: Tuesday, January 29, 2013 12:28:22 PM
> Subject: Re: [Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand
>
> Am 14.01.2013 11:26, schrieb Miroslav Rezanina:
> > This patch adds new qemu-img subcommand that compares content of
> > two disk
> > images.
> >
> > Signed-off-by: Miroslav Rezanina <address@hidden>
> > ---
> > qemu-img-cmds.hx | 6 ++
> > qemu-img.c | 266
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 272 insertions(+), 0 deletions(-)
>
> You also need to add documentation to qemu-img.texi. A description of
> the exit codes should probably go there.
>
As you found out documentation is in patch 4. This code here is needed for
proper working so it's not part of patch 4. I put documentation in separate
patch to have one patch with execution code only. I will add the result codes
in texi file, I forgot them.
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 90b93e0..bc1ccfa 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -27,6 +27,12 @@ STEXI
> > @item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename}
> > ETEXI
> >
> > +DEF("compare", img_compare,
> > + "compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1
> > filename2")
> > +STEXI
> > address@hidden compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s]
> > @var{filename1} @var{filename2}
> > +ETEXI
> > +
> > DEF("convert", img_convert,
> > "convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt]
> > [-o options] [-s snapshot_name] [-S sparse_size] filename
> > [filename2 [...]] output_filename")
> > STEXI
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 8d55829..0c12692 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -693,6 +693,272 @@ static int compare_sectors(const uint8_t
> > *buf1, const uint8_t *buf2, int n,
> >
> > #define IO_BUF_SIZE (2 * 1024 * 1024)
> >
> > +static int64_t sectors_to_bytes(int64_t sectors)
> > +{
> > + return sectors << BDRV_SECTOR_BITS;
> > +}
> > +
> > +static int64_t sectors_to_process(int64_t total, int64_t from)
> > +{
> > + int64_t ret = total - from;
> > +
> > + if (ret > (IO_BUF_SIZE >> BDRV_SECTOR_BITS)) {
> > + return IO_BUF_SIZE >> BDRV_SECTOR_BITS;
> > + }
> > +
> > + return ret;
> > +}
>
> return MIN(total - from, IO_BUF_SIZE >> BDRV_SECTOR_BITS) would be a
> bit
> simpler.
>
> > +
> > +/*
> > + * Check if passed sectors are empty (not allocated or contain
> > only 0 bytes)
> > + *
> > + * Returns 0 in case sectors are filled with 0, 1 if sectors
> > contain non-zero
> > + * data and negative value on error.
> > + *
> > + * @param bs: Driver used for accessing file
> > + * @param sect_num: Number of first sector to check
> > + * @param sect_count: Number of sectors to check
> > + * @param filename: Name of disk file we are checking (logging
> > purpose)
> > + * @param buffer: Allocated buffer for storing read data
> > + * @param quiet: Flag for quiet mode
> > + */
> > +static int check_empty_sectors(BlockDriverState *bs, int64_t
> > sect_num,
> > + int sect_count, const char
> > *filename,
> > + uint8_t *buffer, bool quiet)
> > +{
> > + int pnum, ret = 0;
> > + ret = bdrv_read(bs, sect_num, buffer, sect_count);
> > + if (ret < 0) {
> > + error_report("Error while reading offset %" PRId64 " of
> > %s: %s",
> > + sectors_to_bytes(sect_num), filename,
> > strerror(-ret));
> > + return ret;
> > + }
> > + ret = is_allocated_sectors(buffer, sect_count, &pnum);
> > + if (ret || pnum != sect_count) {
> > + qprintf(quiet, "Content mismatch at offset %" PRId64
> > "!\n",
> > + sectors_to_bytes(ret ? sect_num : sect_num +
> > pnum));
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Compares two images. Exit codes:
> > + *
> > + * 0 - Images are identical
> > + * 1 - Images differ
> > + * 2 - 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
> > */
> > + int progress = 0, quiet = 0, strict = 0;
>
> Please use bool instead.
>
> > + int64_t total_sectors;
> > + int64_t sector_num = 0;
> > + int64_t nb_sectors;
> > + int c, pnum;
> > + uint64_t bs_sectors;
> > + uint64_t progress_base;
> > +
> > + for (;;) {
> > + c = getopt(argc, argv, "hpf:F:sq");
> > + if (c == -1) {
> > + break;
> > + }
> > + switch (c) {
> > + case '?':
> > + case 'h':
> > + help();
> > + break;
> > + case 'f':
> > + fmt1 = optarg;
> > + break;
> > + case 'F':
> > + fmt2 = optarg;
> > + break;
> > + case 'p':
> > + progress = 1;
> > + break;
> > + case 'q':
> > + quiet = 1;
> > + break;
> > + case 's':
> > + strict = 1;
> > + break;
> > + }
> > + }
> > +
> > + /* Progress is not shown in Quiet mode */
> > + if (quiet) {
> > + progress = 0;
> > + }
> > +
> > +
> > + if (optind > argc - 2) {
> > + help();
> > + }
> > + filename1 = argv[optind++];
> > + filename2 = argv[optind++];
> > +
> > + /* Initialize before goto out */
> > + qemu_progress_init(progress, 2.0);
> > +
> > + bs1 = bdrv_new_open(filename1, fmt1, BDRV_O_FLAGS, true,
> > quiet);
> > + if (!bs1) {
> > + error_report("Can't open file %s", filename1);
> > + ret = 2;
> > + goto out3;
> > + }
> > +
> > + bs2 = bdrv_new_open(filename2, fmt2, BDRV_O_FLAGS, true,
> > quiet);
> > + if (!bs2) {
> > + error_report("Can't open file %s", filename2);
> > + ret = 2;
> > + goto out2;
> > + }
> > +
> > + buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> > + buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> > + bdrv_get_geometry(bs1, &bs_sectors);
> > + total_sectors1 = bs_sectors;
> > + bdrv_get_geometry(bs2, &bs_sectors);
> > + total_sectors2 = bs_sectors;
> > + total_sectors = MIN(total_sectors1, total_sectors2);
> > + progress_base = MAX(total_sectors1, total_sectors2);
> > +
> > + qemu_progress_print(0, 100);
> > +
> > + if (strict && total_sectors1 != total_sectors2) {
> > + ret = 1;
> > + qprintf(quiet, "Strict mode: Image size mismatch!\n");
> > + goto out;
> > + }
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(total_sectors,
> > sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + allocated1 = bdrv_is_allocated_above(bs1, NULL,
> > sector_num, nb_sectors,
> > + &pnum1);
> > + allocated2 = bdrv_is_allocated_above(bs2, NULL,
> > sector_num, nb_sectors,
> > + &pnum2);
>
> bdrv_is_allocated_above() can return errors, which need to be
> checked.
>
Right, they have to be checked and respond properly.
> > + nb_sectors = MIN(pnum1, pnum2);
> > +
> > + if (allocated1 == allocated2) {
> > + if (allocated1) {
> > + ret = bdrv_read(bs1, sector_num, buf1,
> > nb_sectors);
> > + if (ret < 0) {
> > + error_report("error while reading offset %"
> > PRId64 " of %s:"
> > + " %s",
> > sectors_to_bytes(sector_num), filename1,
> > + strerror(-ret));
> > + ret = 2;
> > + goto out;
> > + }
> > + ret = bdrv_read(bs2, sector_num, buf2,
> > nb_sectors);
> > + if (ret < 0) {
> > + error_report("error while reading offset %"
> > PRId64
> > + " of %s: %s",
> > sectors_to_bytes(sector_num),
> > + filename2, strerror(-ret));
> > + ret = 2;
> > + goto out;
> > + }
> > + ret = compare_sectors(buf1, buf2, nb_sectors,
> > &pnum);
> > + if (ret || pnum != nb_sectors) {
> > + ret = 1;
> > + qprintf(quiet, "Content mismatch at offset %"
> > PRId64 "!\n",
> > + sectors_to_bytes(
> > + ret ? sector_num : sector_num +
> > pnum));
> > + goto out;
> > + }
> > + }
> > + } else {
> > + if (strict) {
> > + ret = 1;
> > + qprintf(quiet, "Strict mode: Offset %" PRId64
> > + " allocation mismatch!\n",
> > + sectors_to_bytes(sector_num));
> > + goto out;
> > + }
> > +
> > + if (allocated1) {
> > + ret = check_empty_sectors(bs1, sector_num,
> > nb_sectors,
> > + filename1, buf1, quiet);
> > + } else {
> > + ret = check_empty_sectors(bs2, sector_num,
> > nb_sectors,
> > + filename2, buf1, quiet);
> > + }
> > + if (ret) {
> > + if (ret < 0) {
> > + ret = 2;
> > + }
> > + goto out;
> > + }
> > + }
> > + sector_num += nb_sectors;
> > + qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> > + }
> > +
> > + if (total_sectors1 != total_sectors2) {
> > + BlockDriverState *bs_over;
> > + int64_t total_sectors_over;
> > + const char *filename_over;
> > +
> > + qprintf(quiet, "Warning: Image size mismatch!\n");
> > + if (total_sectors1 > total_sectors2) {
> > + total_sectors_over = total_sectors1;
> > + bs_over = bs1;
> > + filename_over = filename1;
> > + } else {
> > + total_sectors_over = total_sectors2;
> > + bs_over = bs2;
> > + filename_over = filename2;
> > + }
> > +
> > + for (;;) {
> > + nb_sectors = sectors_to_process(total_sectors_over,
> > sector_num);
> > + if (nb_sectors <= 0) {
> > + break;
> > + }
> > + ret = bdrv_is_allocated_above(bs_over, NULL,
> > sector_num,
> > + nb_sectors, &pnum);
>
> Error checking again.
>
> > + nb_sectors = pnum;
> > + if (ret) {
> > + ret = check_empty_sectors(bs_over, sector_num,
> > nb_sectors,
> > + filename_over, buf1,
> > quiet);
> > + if (ret) {
> > + if (ret < 0) {
> > + ret = 2;
> > + }
> > + goto out;
> > + }
> > + }
> > + sector_num += nb_sectors;
> > + qemu_progress_print(((float) nb_sectors /
> > progress_base)*100, 100);
> > + }
> > + }
> > +
> > + qprintf(quiet, "Images are identical.\n");
> > + ret = 0;
> > +
> > +out:
> > + bdrv_delete(bs2);
> > + qemu_vfree(buf1);
> > + qemu_vfree(buf2);
> > +out2:
> > + bdrv_delete(bs1);
> > +out3:
> > + qemu_progress_end();
> > + return ret;
> > +}
> > +
> > static int img_convert(int argc, char **argv)
> > {
> > int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
> > cluster_sectors;
> >
>
> Kevin
>
Mirek
--
Miroslav Rezanina
Software Engineer - Virtualization Team
- [Qemu-devel] [PATCH v8 1/4] block: Add synchronous wrapper for bdrv_co_is_allocated_above, (continued)