Patchwork qemu-img rebase: allow backing file to be specified as '-'

login
register
mail settings
Submitter Alex Bligh
Date Oct. 15, 2012, 6:42 p.m.
Message ID <7C9D50488E80F46BDF7A16DA@Ximines.local>
Download mbox | patch
Permalink /patch/191628/
State New
Headers show

Comments

Alex Bligh - Oct. 15, 2012, 6:42 p.m.
Eric,

--On 15 October 2012 12:11:02 -0600 Eric Blake <eblake@redhat.com> wrote:

> On the other hand, if you don't use -u, then qemu-img complains:
>
> $ qemu-img rebase  -b ''  bar
> qemu-img: Could not open new backing file ''
>
> So I think a better patch would be to allow rebase-by-pull to work the
> same as unsafe rebase, by honoring the empty string as a request to pull
> the entire chain into the destination and leave no backing file.

Yes, that's exactly the case we're missing. How about the attached (as
a single patch)

Also available at:
  https://github.com/abligh/qemu
as commits
  https://github.com/abligh/qemu/commit/4cce9c961fa52a71bd6520a9c499f4dc4b174b97
and
  https://github.com/abligh/qemu/commit/4d5b3b431d8dd276f4c564d8a82c6d25cb111381
Eric Blake - Oct. 15, 2012, 7:28 p.m.
On 10/15/2012 12:42 PM, Alex Bligh wrote:
> Eric,
> 
> --On 15 October 2012 12:11:02 -0600 Eric Blake <eblake@redhat.com> wrote:
> 
>> On the other hand, if you don't use -u, then qemu-img complains:
>>
>> $ qemu-img rebase  -b ''  bar
>> qemu-img: Could not open new backing file ''
>>
>> So I think a better patch would be to allow rebase-by-pull to work the
>> same as unsafe rebase, by honoring the empty string as a request to pull
>> the entire chain into the destination and leave no backing file.
> 
> Yes, that's exactly the case we're missing. How about the attached (as
> a single patch)
> 
> Also available at:
>  https://github.com/abligh/qemu
> as commits
>  https://github.com/abligh/qemu/commit/4cce9c961fa52a71bd6520a9c499f4dc4b174b97
> 
> and
>  https://github.com/abligh/qemu/commit/4d5b3b431d8dd276f4c564d8a82c6d25cb111381
> 
> 
> -- 
> Alex Bligh

Missing a Signed-off-by; as such, it cannot be taken as is.  Please
repost, and provide an updated subject line and a dedicated commit
message (information such as where to download the commit for testing
belongs after the --- line).  I'm not maintainer for this portion of
code, so I can only review it.

> @@ -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;

As long as you are touching this line, fix the indentation to be
consistent (or is this unusual spacing an artifact of how you pasted
your commit after your '-- ' signature rather than submitting it via
'git send-email' as a proper patch?).  Also, most of qemu uses spaces on
both sides of '='.

But the overall idea looks nice.
Alex Bligh - Oct. 15, 2012, 8:28 p.m.
Eric,

--On 15 October 2012 13:28:11 -0600 Eric Blake <eblake@redhat.com> wrote:

> Missing a Signed-off-by; as such, it cannot be taken as is.

New patch sent under separate cover with Signed-Off-By: line.

>> @@ -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;
>
> As long as you are touching this line, fix the indentation to be
> consistent (or is this unusual spacing an artifact of how you pasted
> your commit after your '-- ' signature rather than submitting it via
> 'git send-email' as a proper patch?).

I'm totally confused by the spacing artifact. The new patch was generated
with git-format-patch and git-send-email, and never touched a mailer (just
in case), and also shows under the same circumstances. It doesn't show the
bizarre indent in Mulberry, but does in Apple Mail. The patch applies
cleanly with git-am and doesn't moan about whitespace, and as far as I can
tell the resultant whitespace in the file is correct (i.e. matches the
preceding and subsequent lines).

> Also, most of qemu uses spaces on both sides of '='.

Fixed.

> But the overall idea looks nice.

Thanks

Patch

diff --git a/qemu-img.c b/qemu-img.c
index f17f187..7a4e73f 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