Re: [Qemu-devel] [PATCH v3] qemu-img: Implement 'diff' operation.

[Kevin Wolf]

Am 17.05.2012 17:34, schrieb Richard W.M. Jones:
> From: "Richard W.M. Jones" <address@hidden>
> 
> This produces a qcow2 file which is the difference between
> two disk images.  ie, if:
> 
>   base.img     - is a disk image (in any format)
>   modified.img - is base.img, copied and modified
> 
> then:
> 
>   qemu-img diff -b base.img modified.img diff.qcow2
> 
> creates 'diff.qcow2' which contains the differences between 'base.img'
> and 'modified.img'.  Note that 'diff.qcow2' has 'base.img' as its
> backing file.
> 
> Signed-off-by: Richard W.M. Jones <address@hidden>
> Cc: Matthew Booth <address@hidden>
> Cc: Pablo Iranzo Gómez <address@hidden>
> Cc: Tomas Von Veschler <address@hidden>

Hm, I'm wondering... If I have a command line like this:

  qemu-img diff -b base.img modified.img diff.qcow2

Would this be equivalent to this sequence?

  qemu-img create -f qcow2 -b modified.img diff.qcow2
  qemu-img rebase -b base.img diff.qcow2

Or is there some detail that I'm missing? If it is equivalent, this
would suggest that either the new command isn't necessary at all or at
least that it should reuse the qemu-img rebase code.

> +    /* NB: It is possible to relax this constraint so that
> +     * num_sectors_base <= num_sectors_modified, ie. the modified disk
> +     * has been expanded.  That requires changes to the loop below.
> +     */
> +    if (num_sectors_base != num_sectors_modified) {
> +        error_report("Number of sectors in backing and source must be the 
> same");
> +        goto out2;
> +    }

Just to confirm what Eric said, all other places in qemu can (or are
supposed to) deal with backing files that are smaller than the overlay
image.

> +
> +    /* Output image. */
> +    ret = bdrv_img_create(out, fmt_out,
> +                          /* base file becomes the new backing file */
> +                          base, fmt_base,
> +                          options, num_sectors_modified * BDRV_SECTOR_SIZE,
> +                          BDRV_O_FLAGS);
> +    if (ret != 0) {
> +        goto out2;
> +    }
> +    bs_out = bdrv_new_open(out, fmt_out, BDRV_O_RDWR);
> +
> +    buf_base = qemu_blockalign(bs_base, IO_BUF_SIZE);
> +    buf_modified = qemu_blockalign(bs_modified, IO_BUF_SIZE);
> +
> +    for (sector = 0; sector < num_sectors_modified; sector += n) {
> +        /* How many sectors can we handle with the next read? */
> +        if (sector + (IO_BUF_SIZE / BDRV_SECTOR_SIZE) <= 
> num_sectors_modified) {
> +            n = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
> +        } else {
> +            n = num_sectors_modified - sector;
> +        }
> +
> +        /* Read input files and compare. */
> +        ret = bdrv_read(bs_base, sector, buf_base, n);
> +        if (ret < 0) {
> +            error_report("error while reading from backing file");
> +            goto out;

buf_base/modified are leaked here.

> +        }
> +
> +        ret = bdrv_read(bs_modified, sector, buf_modified, n);
> +        if (ret < 0) {
> +            error_report("error while reading from input file");
> +            goto out;

And here.

> +        }
> +
> +        /* If they differ, we need to write to the differences file. */
> +        uint64_t written = 0;
> +
> +        while (written < n) {
> +            int pnum;
> +
> +            if (compare_sectors(buf_base + written * BDRV_SECTOR_SIZE,
> +                                buf_modified + written * BDRV_SECTOR_SIZE,
> +                                n - written, &pnum)) {
> +                ret = bdrv_write(bs_out, sector + written,
> +                                 buf_modified + written * BDRV_SECTOR_SIZE,
> +                                 pnum);
> +                if (ret < 0) {
> +                    error_report("Error while writing to output file: %s",
> +                                 strerror(-ret));
> +                    goto out;

And here.

> +                }
> +            }
> +
> +            written += pnum;
> +        }
> +    }
> +
> +    qemu_vfree(buf_base);
> +    qemu_vfree(buf_modified);
> +
> + out:
> +    /* Cleanup */
> +    bdrv_delete(bs_out);
> + out2:
> +    bdrv_delete(bs_base);
> +    bdrv_delete(bs_modified);
> +
> +    if (ret) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  static int img_resize(int argc, char **argv)
>  {
>      int c, ret, relative;
> diff --git a/qemu-img.texi b/qemu-img.texi
> index b2ca3a5..718d302 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -114,6 +114,28 @@ created as a copy on write image of the specified base 
> image; the
>  @var{backing_file} should have the same content as the input's base image,
>  however the path, image format, etc may differ.
>  
> address@hidden diff [-F @var{backing_fmt}] -b @var{backing_file} [-f 
> @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] @var{filename} 
> @var{output_filename}
> +
> +Create a new disk image (@var{output_filename}) which contains
> +the differences between @var{backing_file} and @var{filename}.
> +This is useful if you have cloned a disk image by copying it,
> +and you want to get back to a thin image on top of a common base.
> +
> +Typical usage is:
> +
> address@hidden diff -b base.img modified.img diff.qcow2}
> +
> +The @var{backing_file} and @var{filename} must have the same
> +virtual disk size, but may be in different formats.
> +
> +The format of @var{output_file} must be one that supports backing
> +files (@code{qcow2} or @code{qed}).

Just don't mention any specific image formats. qcow1, VMDK and COW are
other formats that support backing files and should work just as well.
And if another one is added, nobody will remember to update the qemu-img
documentation.

Kevin