Re: [Qemu-devel] [PATCH] New qemu-img convert -B option to preserve the COW aspect of images and/or re-base them

[Anthony Liguori]

Marc Bevand wrote:
> [PATCH] New qemu-img convert -B option to preserve the COW aspect of
> images and/or re-base them
>
> If a disk image hd_a is a copy-on-write image based on the backing
> file hd_base, it is currently impossible to use qemu-img to convert
> hd_a to hd_b (possibly using another disk image format) while keeping
> hd_b a copy-on-write image of hd_base. qemu-img also doesn't provide a
> feature that would let an enduser re-base a image, for example: adjust
> hd_a's backing file name from hd_base to hd_base2 if it had to change
> for some reason.
>
> This patch solves the 2 above problems by adding a new qemu-img
> convert -B option. This is a generic feature that should work with
> ANY disk image format supporting backing files. Examples:
>
>   $ qemu-img info hd_a
>   image: hd_a
>   file format: qcow
>   virtual size: 6.0G (6442450944 bytes)
>   disk size: 28K
>   cluster_size: 512
>   backing file: hd_base (actual path: hd_base)
>
> Converting hd_a (qcow) to hd_b (qcow2) while preserving the
> copy-on-write aspect of the image:
>
>   $ qemu-img convert hd_a -O qcow2 -B hd_base hd_b
>   $ qemu-img info hd_b
>   image: hd_b
>   file format: qcow2
>   virtual size: 6.0G (6442450944 bytes)
>   disk size: 36K
>   cluster_size: 4096
>   backing file: hd_base (actual path: hd_base)
>
> Renaming the backing file without losing hd_a:
>
>   $ ln hd_base hd_base2
>   $ qemu-img convert hd_a -O qcow -B hd_base2 hd_a2
>   $ mv hd_a2 hd_a
>   $ rm hd_base
>   $ qemu-img info hd_a
>   image: hd_a
>   file format: qcow
>   virtual size: 6.0G (6442450944 bytes)
>   disk size: 28K
>   cluster_size: 512
>   backing file: hd_base2 (actual path: hd_base2)
>
> Patch made against SVN's rev 4622.
>
>
> Signed-off-by: Marc Bevand <m.bevand <at> gmail.com>
>
> Index: qemu-img.texi
> ===================================================================
> --- qemu-img.texi       (revision 4622)
> +++ qemu-img.texi       (working copy)
> @@ -10,7 +10,7 @@
>  @table @option
>  @item create [-e] [-6] [-b @var{base_image}] [-f @var{fmt}]
> @var{filename} address@hidden
>  @item commit [-f @var{fmt}] @var{filename}
> address@hidden convert [-c] [-e] [-6] [-f @var{fmt}] @var{filename} [-O
> @var{output_fmt}] @var{output_filename}
> address@hidden convert [-c] [-e] [-6] [-f @var{fmt}] [-O @var{output_fmt}] [-B
> @var{output_base_image}] @var{filename} address@hidden [...]]
>   

Please don't mix adding a new option with changing the format of the 
help text.

> @var{output_filename}
>  @item info [-f @var{fmt}] @var{filename}
>  @end table
>
> @@ -21,7 +21,11 @@
>  @item base_image
>  is the read-only disk image which is used as base for a copy on
>      write image; the copy on write image only stores the modified data
> -
> address@hidden output_base_image
> +forces the output image to be created as a copy on write
> +image of the specified base image; @code{output_base_image} should
> have the same
> +content as the input's base image, however the path, image format, etc may
> +differ
>  @item fmt
>  is the disk image format. It is guessed automatically in most cases.
> The following formats are supported:
>
> Index: block.c
> ===================================================================
> --- block.c     (revision 4622)
> +++ block.c     (working copy)
> @@ -884,6 +884,32 @@
>          bdrv_flush(bs->backing_hd);
>  }
>
> +/*
> + * Returns true iff the specified sector is present in the disk image. Drivers
> + * not implementing the functionality are assumed to not support backing files,
> + * hence all their sectors are reported as allocated.
> + *
> + * 'pnum' is set to the number of sectors (including and immediately following
> + * the specified sector) that are known to be in the same
> + * allocated/unallocated state.
> + *
> + * 'nb_sectors' is the max value 'pnum' should be set to.
> + */
> +int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> +       int *pnum)
> +{
> +    if (!bs->drv->bdrv_is_allocated) {
> +        if (sector_num >= bs->total_sectors) {
> +            *pnum = 0;
> +            return 0;
> +        }
> +        int64_t n = bs->total_sectors - sector_num;
>   

Don't mix declarations with code.

> +        *pnum = (n < nb_sectors) ? (n) : (nb_sectors);
> +        return 1;
> +    }
> +    return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
> +}
> +
>  #ifndef QEMU_IMG
>  void bdrv_info(void)
>  {
> Index: block.h
> ===================================================================
> --- block.h     (revision 4622)
> +++ block.h     (working copy)
> @@ -99,6 +99,8 @@
>
>  /* Ensure contents are flushed to disk.  */
>  void bdrv_flush(BlockDriverState *bs);
> +int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> +       int *pnum);
>
>  #define BDRV_TYPE_HD     0
>  #define BDRV_TYPE_CDROM  1
> Index: qemu-img.c
> ===================================================================
> --- qemu-img.c  (revision 4622)
> +++ qemu-img.c  (working copy)
> @@ -55,13 +55,17 @@
>             "Command syntax:\n"
>             "  create [-e] [-6] [-b base_image] [-f fmt] filename [size]\n"
>             "  commit [-f fmt] filename\n"
> -           "  convert [-c] [-e] [-6] [-f fmt] [-O output_fmt]
> filename [filename2 [...]] output_filename\n"
> +           "  convert [-c] [-e] [-6] [-f fmt] [-O output_fmt] [-B
> output_base_image] filename [filename2 [...]] output_filename\n"
>   

Please fix your mailer.  It's impossible to review with the whitespace 
damage and I can't comment on something downloaded from a website.

Regards,

Anthony Liguori