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

[Eric Blake]

On 05/17/2012 08:57 AM, Richard W.M. Jones wrote:
> 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.
> 

> +++ b/qemu-img-cmds.hx
> @@ -33,6 +33,12 @@ STEXI
>  @item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O 
> @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S 
> @var{sparse_size}] @var{filename} address@hidden [...]] @var{output_filename}
>  ETEXI
>  
> +DEF("diff", img_diff,
> +    "diff [-F backing_fmt] -b backing_file [-f fmt] [-O output_fmt] [-o 
> options] filename output_filename")
> +STEXI
> 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}

Why the difference in ordering between -o and -O?


> +
> +    if (argc - optind != 2) {
> +        error_report("The input and output filenames must be supplied");
> +        return 1;

Error is misleading if argc > optind+2.


> +
> +    if (fmt_out == NULL || fmt_out[0] == '\0') {
> +        fmt_out = "qcow2";
> +    }

So output defaults to qcow2, and input (both base and modified) default
to probing.  Works for me.

> +
> +    bdrv_get_geometry(bs_base, &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;
> +    }

I can live with an initial implementation that is strict, where a later
patch relaxes things to allow a modified image that has been enlarged.

> +
> +    /* Output image. */
> +    ret = bdrv_img_create(out, fmt_out,
> +                          /* base file becomes the new backing file */
> +                          base, fmt_base,
> +                          options,
> +                          num_sectors * BDRV_SECTOR_SIZE, BDRV_O_FLAGS);

I still think this should be modified_num_sectors - for now, the two
values are equal, but if we relax the error check up above, then you
really do want to go with the output file the same size as the modified
file.

It looks like you did indeed address most of my comments on v1.

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature