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

login
register
mail settings
Submitter Alex Bligh
Date Oct. 16, 2012, 12:46 p.m.
Message ID <1350391578-1191-1-git-send-email-alex@alex.org.uk>
Download mbox | patch
Permalink /patch/191798/
State New
Headers show

Comments

Alex Bligh - Oct. 16, 2012, 12:46 p.m.
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    |   29 +++++++++++++++++++----------
 qemu-img.texi |    4 +++-
 2 files changed, 22 insertions(+), 11 deletions(-)

Also obtainable from:
  https://github.com/abligh/qemu.git
Commit at:
  https://github.com/abligh/qemu/commit/982ec105018f9901619663642de9d0a5de86799d
Kevin Wolf - Oct. 16, 2012, 12:54 p.m.
Am 16.10.2012 14:46, 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>

Thanks, applied to the block branch.

Kevin
Kevin Wolf - Oct. 17, 2012, 2:45 p.m.
Am 16.10.2012 14:46, 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    |   29 +++++++++++++++++++----------
>  qemu-img.texi |    4 +++-
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> Also obtainable from:
>   https://github.com/abligh/qemu.git
> Commit at:
>   https://github.com/abligh/qemu/commit/982ec105018f9901619663642de9d0a5de86799d
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f17f187..2816f37 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1558,13 +1558,15 @@ 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 +1582,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 +1594,9 @@ 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 +1633,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 +1679,12 @@ 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) {

I think this needs to be out_baseimg, otherwise -u is broken. I've
updated the patch, please check if you agree with the fix.

Kevin
Alex Bligh - Oct. 18, 2012, 9:20 p.m.
Kevin,

--On 17 October 2012 16:45:39 +0200 Kevin Wolf <kwolf@redhat.com> wrote:

> um_sectors) {
>> @@ -1675,7 +1679,12 @@ 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) {
>
> I think this needs to be out_baseimg, otherwise -u is broken. I've
> updated the patch, please check if you agree with the fix.

I'm not sure I do agree.

When -u is not specified, then unsafe=0. If the backing file is the empty
string then bs_new_backing is 0 here, and the if condition evaluates to
false, in the current patch.

If you make that "if (outbase_img)" then it will still evaluate to true,
because whilst outbase_img is non-zero, outbase_img[0] is zero.

So I think you either need to do:

   if (bs_new_backing || unsafe)

which replicates the existing behaviour, or

   if (out_baseimg && out_baseimg[0])


As it happens, we despite what Eric Blake said, we couldn't get an unsafe
rebase to no backing file to work with the existing code (with our without
our patch). The second option may fix this bug. Reading line 1497, is this
because the semantic is not 'an empty string', but 'omit -b entirely'?
This behaviour is undocumented in the manpage which specifies -b as a
compulsory option. If so, that's a bit unfortunate as we now have different
semantics with and without -u. Note if no -b parameter is supplied, there
is also a possible null pointer exception at line 1693 (null passed to
error_report).
Kevin Wolf - Oct. 19, 2012, 4:46 p.m.
Am 18.10.2012 23:20, schrieb Alex Bligh:
> Kevin,
> 
> --On 17 October 2012 16:45:39 +0200 Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> um_sectors) {
>>> @@ -1675,7 +1679,12 @@ 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) {
>>
>> I think this needs to be out_baseimg, otherwise -u is broken. I've
>> updated the patch, please check if you agree with the fix.
> 
> I'm not sure I do agree.
> 
> When -u is not specified, then unsafe=0. If the backing file is the empty
> string then bs_new_backing is 0 here, and the if condition evaluates to
> false, in the current patch.
> 
> If you make that "if (outbase_img)" then it will still evaluate to true,
> because whilst outbase_img is non-zero, outbase_img[0] is zero.
> 
> So I think you either need to do:
> 
>    if (bs_new_backing || unsafe)
> 
> which replicates the existing behaviour, or
> 
>    if (out_baseimg && out_baseimg[0])

Good point, I changed it.

> As it happens, we despite what Eric Blake said, we couldn't get an unsafe
> rebase to no backing file to work with the existing code (with our without
> our patch). The second option may fix this bug. Reading line 1497, is this
> because the semantic is not 'an empty string', but 'omit -b entirely'?
> This behaviour is undocumented in the manpage which specifies -b as a
> compulsory option. If so, that's a bit unfortunate as we now have different
> semantics with and without -u. Note if no -b parameter is supplied, there
> is also a possible null pointer exception at line 1693 (null passed to
> error_report).

Right. I think not passing -b at all or passing an empty string should
have the same meaning, namely removing the backing file reference. I
won't try to modify this patch to do this, though, we can do it on top.

Kevin
Eric Blake - Oct. 19, 2012, 5:08 p.m.
On 10/19/2012 10:46 AM, Kevin Wolf wrote:
>> As it happens, we despite what Eric Blake said, we couldn't get an unsafe
>> rebase to no backing file to work with the existing code (with our without
>> our patch). The second option may fix this bug. Reading line 1497, is this
>> because the semantic is not 'an empty string', but 'omit -b entirely'?
>> This behaviour is undocumented in the manpage which specifies -b as a
>> compulsory option. If so, that's a bit unfortunate as we now have different
>> semantics with and without -u. Note if no -b parameter is supplied, there
>> is also a possible null pointer exception at line 1693 (null passed to
>> error_report).
> 
> Right. I think not passing -b at all or passing an empty string should
> have the same meaning, namely removing the backing file reference. I
> won't try to modify this patch to do this, though, we can do it on top.

Agreed - a future patch that makes -b optional rather than mandatory,
where '-b ""' and omitting the argument have the same semantics whether
or not -u is present, would be nice.

Patch

diff --git a/qemu-img.c b/qemu-img.c
index f17f187..2816f37 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1558,13 +1558,15 @@  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 +1582,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 +1594,9 @@  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 +1633,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 +1679,12 @@  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