diff mbox series

[1/3] qemu-img: Allow rebase with no input base

Message ID 20180713111424.23670-2-mreitz@redhat.com
State New
Headers show
Series qemu-img: Allow rebase with no input base | expand

Commit Message

Max Reitz July 13, 2018, 11:14 a.m. UTC 
Currently, you cannot add a backing file to an image when it currently
has none.  It is really simple to allow this, though (effectively by
setting old_backing_size to 0), so this patch does just that.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

Comments

Eric Blake April 18, 2019, 6:55 p.m. UTC | #1 
On 7/13/18 6:14 AM, Max Reitz wrote:
> Currently, you cannot add a backing file to an image when it currently
> has none.  It is really simple to allow this, though (effectively by
> setting old_backing_size to 0), so this patch does just that.

Can't you do that with 'rebase -u'? I guess the point of this patch is
to make 'rebase' without -u also able to add a backing file?  A bit more
clarity in the commit message, such as a sample command line (and
possibly even the error it issued pre-patch) would help.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)

At any rate, the code looks fine, even if the diff is harder to read
because of added indentation.

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz April 24, 2019, 1:38 p.m. UTC | #2 
On 18.04.19 20:55, Eric Blake wrote:
> On 7/13/18 6:14 AM, Max Reitz wrote:
>> Currently, you cannot add a backing file to an image when it currently
>> has none.  It is really simple to allow this, though (effectively by
>> setting old_backing_size to 0), so this patch does just that.
> 
> Can't you do that with 'rebase -u'? I guess the point of this patch is
> to make 'rebase' without -u also able to add a backing file?  A bit more
> clarity in the commit message, such as a sample command line (and
> possibly even the error it issued pre-patch) would help.

Yes, this is about normal rebase without -u.

There is an example in the cover letter, but I can add it to this patch
here, too, yes.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
>>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> At any rate, the code looks fine, even if the diff is harder to read
> because of added indentation.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max
diff mbox series

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..dd684d8bf0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3280,26 +3280,30 @@  static int img_rebase(int argc, char **argv)
         char backing_name[PATH_MAX];
         QDict *options = NULL;
 
-        if (bs->backing_format[0] != '\0') {
-            options = qdict_new();
-            qdict_put_str(options, "driver", bs->backing_format);
-        }
-
-        if (force_share) {
-            if (!options) {
+        if (bs->backing) {
+            if (bs->backing_format[0] != '\0') {
                 options = qdict_new();
+                qdict_put_str(options, "driver", bs->backing_format);
             }
-            qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
-        }
-        bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        blk_old_backing = blk_new_open(backing_name, NULL,
-                                       options, src_flags, &local_err);
-        if (!blk_old_backing) {
-            error_reportf_err(local_err,
-                              "Could not open old backing file '%s': ",
-                              backing_name);
-            ret = -1;
-            goto out;
+
+            if (force_share) {
+                if (!options) {
+                    options = qdict_new();
+                }
+                qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
+            }
+            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
+            blk_old_backing = blk_new_open(backing_name, NULL,
+                                           options, src_flags, &local_err);
+            if (!blk_old_backing) {
+                error_reportf_err(local_err,
+                                  "Could not open old backing file '%s': ",
+                                  backing_name);
+                ret = -1;
+                goto out;
+            }
+        } else {
+            blk_old_backing = NULL;
         }
 
         if (out_baseimg[0]) {
@@ -3355,7 +3359,7 @@  static int img_rebase(int argc, char **argv)
      */
     if (!unsafe) {
         int64_t size;
-        int64_t old_backing_size;
+        int64_t old_backing_size = 0;
         int64_t new_backing_size = 0;
         uint64_t offset;
         int64_t n;
@@ -3371,15 +3375,18 @@  static int img_rebase(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        old_backing_size = blk_getlength(blk_old_backing);
-        if (old_backing_size < 0) {
-            char backing_name[PATH_MAX];
+        if (blk_old_backing) {
+            old_backing_size = blk_getlength(blk_old_backing);
+            if (old_backing_size < 0) {
+                char backing_name[PATH_MAX];
 
-            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-            error_report("Could not get size of '%s': %s",
-                         backing_name, strerror(-old_backing_size));
-            ret = -1;
-            goto out;
+                bdrv_get_backing_filename(bs, backing_name,
+                                          sizeof(backing_name));
+                error_report("Could not get size of '%s': %s",
+                             backing_name, strerror(-old_backing_size));
+                ret = -1;
+                goto out;
+            }
         }
         if (blk_new_backing) {
             new_backing_size = blk_getlength(blk_new_backing);