[PATCHv3] qemu-img rebase: use empty string to rebase without backing file

This patch allows an empty filename to be passed as the new base image name
for qemu-img rebase to mean base the image on no backing file (i.e.
independent of any backing file). According to Eric Blake, qemu-img rebase
already supports this when '-u' is used; this adds support when -u is not
used.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 qemu-img.c    |   26 ++++++++++++++++----------
 qemu-img.texi |    4 +++-
 2 files changed, 19 insertions(+), 11 deletions(-)

Also obtainable from:
  https://github.com/abligh/qemu.git
Commit at:
  https://github.com/abligh/qemu/commit/49cd454aa09062b151710cc7afd4bb7fcf1070d0

On 10/15/2012 02:50 PM, Alex Bligh wrote:
> This patch allows an empty filename to be passed as the new base image name
> for qemu-img rebase to mean base the image on no backing file (i.e.
> independent of any backing file). According to Eric Blake, qemu-img rebase
> already supports this when '-u' is used; this adds support when -u is not
> used.
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  qemu-img.c    |   26 ++++++++++++++++----------
>  qemu-img.texi |    4 +++-
>  2 files changed, 19 insertions(+), 11 deletions(-)

Per http://wiki.qemu.org/Contribute/SubmitAPatch, it helps to add a
maintainer in cc; I'm guessing that this is most closely related to
block work, and therefore adding Kevin in cc.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> Also obtainable from:
>   https://github.com/abligh/qemu.git
> Commit at:
>   https://github.com/abligh/qemu/commit/49cd454aa09062b151710cc7afd4bb7fcf1070d0
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f17f187..e817498 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1558,13 +1558,14 @@ static int img_rebase(int argc, char **argv)
>              error_report("Could not open old backing file '%s'", backing_name);
>              goto out;
>          }
> -
> -        bs_new_backing = bdrv_new("new_backing");
> -        ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS,
> +        if (out_baseimg[0]) {
> +            bs_new_backing = bdrv_new("new_backing");
> +            ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS,
>                          new_backing_drv);
> -        if (ret) {
> -            error_report("Could not open new backing file '%s'", out_baseimg);
> -            goto out;
> +            if (ret) {
> +                error_report("Could not open new backing file '%s'", out_baseimg);
> +                goto out;
> +            }
>          }
>      }
>  
> @@ -1580,7 +1581,7 @@ static int img_rebase(int argc, char **argv)
>      if (!unsafe) {
>          uint64_t num_sectors;
>          uint64_t old_backing_num_sectors;
> -        uint64_t new_backing_num_sectors;
> +        uint64_t new_backing_num_sectors = 0;
>          uint64_t sector;
>          int n;
>          uint8_t * buf_old;
> @@ -1592,7 +1593,8 @@ static int img_rebase(int argc, char **argv)
>  
>          bdrv_get_geometry(bs, &num_sectors);
>          bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
> -        bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
> +        if (bs_new_backing)
> +            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
>  
>          local_progress = (float)100 /
>              (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
> @@ -1629,7 +1631,7 @@ static int img_rebase(int argc, char **argv)
>                  }
>              }
>  
> -            if (sector >= new_backing_num_sectors) {
> +            if (sector >= new_backing_num_sectors || !bs_new_backing) {
>                  memset(buf_new, 0, n * BDRV_SECTOR_SIZE);
>              } else {
>                  if (sector + n > new_backing_num_sectors) {
> @@ -1675,7 +1677,11 @@ static int img_rebase(int argc, char **argv)
>       * backing file are overwritten in the COW file now, so the visible content
>       * doesn't change when we switch the backing file.
>       */
> -    ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
> +    if (bs_new_backing)
> +        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
> +    else
> +        ret = bdrv_change_backing_file(bs, NULL, NULL);
> +
>      if (ret == -ENOSPC) {
>          error_report("Could not change the backing file to '%s': No "
>                       "space left in the file header", out_baseimg);
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 8b05f2c..42ec392 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -148,7 +148,9 @@ Changes the backing file of an image. Only the formats @code{qcow2} and
>  
>  The backing file is changed to @var{backing_file} and (if the image format of
>  @var{filename} supports this) the backing file format is changed to
> -@var{backing_fmt}.
> +@var{backing_fmt}. If @var{backing_file} is specified as ``'' (the empty
> +string), then the image is rebased onto no backing file (i.e. it will exist
> +independently of any backing file).
>  
>  There are two different modes in which @code{rebase} can operate:
>  @table @option
>

Am 15.10.2012 22:50, schrieb Alex Bligh:
> This patch allows an empty filename to be passed as the new base image name
> for qemu-img rebase to mean base the image on no backing file (i.e.
> independent of any backing file). According to Eric Blake, qemu-img rebase
> already supports this when '-u' is used; this adds support when -u is not
> used.
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  qemu-img.c    |   26 ++++++++++++++++----------
>  qemu-img.texi |    4 +++-
>  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> Also obtainable from:
>   https://github.com/abligh/qemu.git
> Commit at:
>   https://github.com/abligh/qemu/commit/49cd454aa09062b151710cc7afd4bb7fcf1070d0
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f17f187..e817498 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1558,13 +1558,14 @@ static int img_rebase(int argc, char **argv)
>              error_report("Could not open old backing file '%s'", backing_name);
>              goto out;
>          }
> -
> -        bs_new_backing = bdrv_new("new_backing");
> -        ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS,
> +        if (out_baseimg[0]) {
> +            bs_new_backing = bdrv_new("new_backing");
> +            ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS,
>                          new_backing_drv);
> -        if (ret) {
> -            error_report("Could not open new backing file '%s'", out_baseimg);
> -            goto out;
> +            if (ret) {
> +                error_report("Could not open new backing file '%s'", out_baseimg);

This exceeds 80 characters per line and must be wrapped. (Please use
scripts/checkpatch.pl to check for some common coding style errors)

> +                goto out;
> +            }
>          }
>      }
>  
> @@ -1580,7 +1581,7 @@ static int img_rebase(int argc, char **argv)
>      if (!unsafe) {
>          uint64_t num_sectors;
>          uint64_t old_backing_num_sectors;
> -        uint64_t new_backing_num_sectors;
> +        uint64_t new_backing_num_sectors = 0;
>          uint64_t sector;
>          int n;
>          uint8_t * buf_old;
> @@ -1592,7 +1593,8 @@ static int img_rebase(int argc, char **argv)
>  
>          bdrv_get_geometry(bs, &num_sectors);
>          bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
> -        bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
> +        if (bs_new_backing)
> +            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);

Please add braces here.

>  
>          local_progress = (float)100 /
>              (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
> @@ -1629,7 +1631,7 @@ static int img_rebase(int argc, char **argv)
>                  }
>              }
>  
> -            if (sector >= new_backing_num_sectors) {
> +            if (sector >= new_backing_num_sectors || !bs_new_backing) {
>                  memset(buf_new, 0, n * BDRV_SECTOR_SIZE);
>              } else {
>                  if (sector + n > new_backing_num_sectors) {
> @@ -1675,7 +1677,11 @@ static int img_rebase(int argc, char **argv)
>       * backing file are overwritten in the COW file now, so the visible content
>       * doesn't change when we switch the backing file.
>       */
> -    ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
> +    if (bs_new_backing)
> +        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
> +    else
> +        ret = bdrv_change_backing_file(bs, NULL, NULL);

Braces here as well.

Apart from the coding style problems, the patch looks good to me.

I think it would also be interesting to do the symmetrical thing in a
follow-up patch, namely rebasing an image that doesn't have a backing
file yet. This will be useful when the TODO is addressed and clusters
present in the backing file can be dropped. Then you could convert
modified copies of an image back into deltas. (I'm not requesting that
you implement this, just thinking aloud :-))

Kevin

Kevin,

--On 16 October 2012 13:22:47 +0200 Kevin Wolf <kwolf@redhat.com> wrote:

> Apart from the coding style problems, the patch looks good to me.

Coding problems fixed & v4 sent under separate cover.