All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
@ 2012-10-16 12:46 Alex Bligh
  2012-10-16 12:54 ` Kevin Wolf
  2012-10-17 14:45 ` Kevin Wolf
  0 siblings, 2 replies; 6+ messages in thread
From: Alex Bligh @ 2012-10-16 12:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, alex

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) {
+        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
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
  2012-10-16 12:46 [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file Alex Bligh
@ 2012-10-16 12:54 ` Kevin Wolf
  2012-10-17 14:45 ` Kevin Wolf
  1 sibling, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2012-10-16 12:54 UTC (permalink / raw)
  To: Alex Bligh; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
  2012-10-16 12:46 [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file Alex Bligh
  2012-10-16 12:54 ` Kevin Wolf
@ 2012-10-17 14:45 ` Kevin Wolf
  2012-10-18 21:20   ` Alex Bligh
  1 sibling, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2012-10-17 14:45 UTC (permalink / raw)
  To: Alex Bligh; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
  2012-10-17 14:45 ` Kevin Wolf
@ 2012-10-18 21:20   ` Alex Bligh
  2012-10-19 16:46     ` Kevin Wolf
  0 siblings, 1 reply; 6+ messages in thread
From: Alex Bligh @ 2012-10-18 21:20 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, 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])


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).

-- 
Alex Bligh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
  2012-10-18 21:20   ` Alex Bligh
@ 2012-10-19 16:46     ` Kevin Wolf
  2012-10-19 17:08       ` Eric Blake
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2012-10-19 16:46 UTC (permalink / raw)
  To: Alex Bligh; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file
  2012-10-19 16:46     ` Kevin Wolf
@ 2012-10-19 17:08       ` Eric Blake
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2012-10-19 17:08 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, Alex Bligh

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-10-19 17:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-16 12:46 [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file Alex Bligh
2012-10-16 12:54 ` Kevin Wolf
2012-10-17 14:45 ` Kevin Wolf
2012-10-18 21:20   ` Alex Bligh
2012-10-19 16:46     ` Kevin Wolf
2012-10-19 17:08       ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.