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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3445 bytes --]

On 05/17/2012 07:44 AM, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
> This produces a qcow2 file which is the different between
> two disk images.  ie, if:
> 
>   original.img - is a disk image (in any format)
>   modified.img - is a modified version of original.img
> 
> then:
> 
>   qemu-img diff -b original.img modified.img diff.qcow2
> 
> creates 'diff.qcow2' which contains just the differences.  Note that
> 'diff.qcow2' has 'original.img' set as the backing file.

Sounds useful!

>  
> +DEF("diff", img_diff,
> +    "diff [-f fmt] [-F backing_fmt] [-O output_fmt] -b backing_file filename output_filename")

Just so I'm clear: -f is for filename (the file with the modifications
being extracted), -F is for backing_file (the file that serves as the
base of the diff), and -O is for output_filename.

> +STEXI
> +@item rebase [-f @var{fmt}] [-F @var{backing_fmt}] [-O @var{output_fmt}] -b @var{backing_file} @var{filename} @var{output_filename}

s/rebase/diff/

We also need to support -o options for the output_filename, so that we
can expose other qcow2 attributes while creating the diff.  For example,
encryption, cluster_size, and preacllocation all come to mind.


> +
> +    bdrv_get_geometry(bs_original, &num_sectors);
> +    bdrv_get_geometry(bs_modified, &modified_num_sectors);
> +    if (num_sectors != modified_num_sectors) {
> +        error_report("Number of sectors in backing and source must be the same");
> +        goto out2;
> +    }

Why are you requiring equality?  I can see the usefulness of doing a
diff where the modified file is larger than the original (basically, the
diff was created by extending the original file to something larger).
Prohibiting a modified file smaller than the original makes sense, so I
think this should be >, not !=.

> +
> +    /* Output image. */
> +    if (fmt_out == NULL || fmt_out[0] == '\0') {
> +        fmt_out = "qcow2";
> +    }
> +    ret = bdrv_img_create(out, fmt_out,
> +                          /* original file becomes the new backing file */
> +                          original, fmt_original,
> +                          NULL, num_sectors * BDRV_SECTOR_SIZE, BDRV_O_FLAGS);

If you allow a modified larger than backing, then this should be
modified_num_sectors, not num_sectors.


> +++ b/qemu-img.texi
> @@ -114,6 +114,23 @@ 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.
>  
> +@item diff [-f @var{fmt}] [-F @var{backing_fmt}] [-O @var{output_fmt}] -b @var{backing_file} @var{filename} @var{output_filename}
> +
> +Create a new file (@var{output_filename}) which contains the
> +differences between @var{backing_file} and @var{filename}.
> +
> +The @var{backing_file} and @var{filename} must have the same
> +virtual disk size, but may be in different formats.

Again, I think this is overly tight.

> +
> +@var{output_file} will have @var{backing_file} set as its backing
> +file.  The format of @var{output_file} must be one that supports
> +backing files (currently @code{qcow2} is the default and only
> +permitted output format).

Why doesn't qed just work out of the box?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]