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

[Kevin Wolf]

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 <address@hidden>
> ---
>  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