All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
@ 2012-10-15 17:29 Alex Bligh
  2012-10-15 18:07 ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Bligh @ 2012-10-15 17:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alex Bligh

This patch allows qemu-img rebase to rebase an image to
have no backing file, as opposed to merely allowing it to
rebase to an existing backing file.

Patch below, or pull from git at:
  https://github.com/abligh/qemu.git

Commit visible at:
https://github.com/abligh/qemu/commit/4d5b3b431d8dd276f4c564d8a82c6d25cb111381


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

diff --git a/qemu-img.c b/qemu-img.c
index f17f187..770e221 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 (strcmp("-", out_baseimg)) {
+            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..7c29f23 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 ``-'', 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.0.4

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

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 17:29 [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-' Alex Bligh
@ 2012-10-15 18:07 ` Eric Blake
  2012-10-15 18:11   ` Eric Blake
  2012-10-23 16:41   ` Christoph Hellwig
  0 siblings, 2 replies; 7+ messages in thread
From: Eric Blake @ 2012-10-15 18:07 UTC (permalink / raw)
  To: Alex Bligh; +Cc: qemu-devel

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

On 10/15/2012 11:29 AM, Alex Bligh wrote:
> This patch allows qemu-img rebase to rebase an image to
> have no backing file, as opposed to merely allowing it to
> rebase to an existing backing file.

You can already do that by rebasing to the empty string.  And it is
feasible (although unlikely) to have a file named '-', where your patch
would make it impossible to use that file directly (although you could
still use './-').  NACK.

$ qemu-img info bar
image: bar
file format: qcow2
virtual size: 0 (0 bytes)
disk size: 192K
cluster_size: 65536
backing file: foo
$ qemu-img rebase -u -b ''  bar
$ qemu-img info bar
image: bar
file format: qcow2
virtual size: 0 (0 bytes)
disk size: 192K
cluster_size: 65536


-- 
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] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 18:07 ` Eric Blake
@ 2012-10-15 18:11   ` Eric Blake
  2012-10-15 18:42     ` Alex Bligh
  2012-10-23 16:41   ` Christoph Hellwig
  1 sibling, 1 reply; 7+ messages in thread
From: Eric Blake @ 2012-10-15 18:11 UTC (permalink / raw)
  Cc: qemu-devel, Alex Bligh

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

On 10/15/2012 12:07 PM, Eric Blake wrote:
> On 10/15/2012 11:29 AM, Alex Bligh wrote:
>> This patch allows qemu-img rebase to rebase an image to
>> have no backing file, as opposed to merely allowing it to
>> rebase to an existing backing file.
> 
> You can already do that by rebasing to the empty string.  And it is
> feasible (although unlikely) to have a file named '-', where your patch
> would make it impossible to use that file directly (although you could
> still use './-').  NACK.
> 
> $ qemu-img info bar
> image: bar
> file format: qcow2
> virtual size: 0 (0 bytes)
> disk size: 192K
> cluster_size: 65536
> backing file: foo
> $ qemu-img rebase -u -b ''  bar
> $ qemu-img info bar
> image: bar
> file format: qcow2
> virtual size: 0 (0 bytes)
> disk size: 192K
> cluster_size: 65536

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.

-- 
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] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 18:11   ` Eric Blake
@ 2012-10-15 18:42     ` Alex Bligh
  2012-10-15 19:28       ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Bligh @ 2012-10-15 18:42 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, Alex Bligh

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

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

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

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 18:42     ` Alex Bligh
@ 2012-10-15 19:28       ` Eric Blake
  2012-10-15 20:28         ` Alex Bligh
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2012-10-15 19:28 UTC (permalink / raw)
  To: Alex Bligh; +Cc: qemu-devel

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

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.

-- 
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] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 19:28       ` Eric Blake
@ 2012-10-15 20:28         ` Alex Bligh
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Bligh @ 2012-10-15 20:28 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, Alex Bligh

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

-- 
Alex Bligh

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

* Re: [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-'
  2012-10-15 18:07 ` Eric Blake
  2012-10-15 18:11   ` Eric Blake
@ 2012-10-23 16:41   ` Christoph Hellwig
  1 sibling, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2012-10-23 16:41 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, Alex Bligh

On Mon, Oct 15, 2012 at 12:07:37PM -0600, Eric Blake wrote:
> On 10/15/2012 11:29 AM, Alex Bligh wrote:
> > This patch allows qemu-img rebase to rebase an image to
> > have no backing file, as opposed to merely allowing it to
> > rebase to an existing backing file.
> 
> You can already do that by rebasing to the empty string.  And it is
> feasible (although unlikely) to have a file named '-', where your patch
> would make it impossible to use that file directly (although you could
> still use './-').  NACK.

Also - is often used to tell tools that expect a file argument to use
stdin/out.  Something that would be quite useful for some qemu-img subcommands.

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

end of thread, other threads:[~2012-10-23 16:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-15 17:29 [Qemu-devel] [PATCH] qemu-img rebase: allow backing file to be specified as '-' Alex Bligh
2012-10-15 18:07 ` Eric Blake
2012-10-15 18:11   ` Eric Blake
2012-10-15 18:42     ` Alex Bligh
2012-10-15 19:28       ` Eric Blake
2012-10-15 20:28         ` Alex Bligh
2012-10-23 16:41   ` Christoph Hellwig

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.