[PULL,13/15] qemu-img: Allow rebase with no input base

Commit Message

Kevin Wolf May 10, 2019, 4:16 p.m. UTC

From: Max Reitz <[e-mail]>

Currently, without -u, you cannot add a backing file to an image when it
currently has none:

$ qemu-img rebase -b base.qcow2 foo.qcow2
qemu-img: Could not open old backing file '': The 'file' block driver
requires a file name

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 <[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>
Signed-off-by: Kevin Wolf <[e-mail]>
---
 qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

Comments

Peter Maydell May 20, 2019, 3:33 p.m. UTC | #1

On Fri, 10 May 2019 at 17:37, Kevin Wolf <[e-mail]> wrote:
>
> From: Max Reitz <[e-mail]>
>
> Currently, without -u, you cannot add a backing file to an image when it
> currently has none:
>
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver
> requires a file name
>
> 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 <[e-mail]>
> Reviewed-by: Eric Blake <[e-mail]>
> Signed-off-by: Kevin Wolf <[e-mail]>

Hi -- Coverity reports some new "resource leak" errors
in the qemu-img.c img_rebase() function:
 * CID 1401415 : Coverity complains that the memory assigned
   to out_real_path gets leaked if we take the immediately
   following "if (local_err) ... goto out" error-exit path.
   This may be another instance of Coverity not understanding
   that the function won't return non-NULL if errp is set.
 * CID 1401416 : leak of the 'options' qdict in this
   error-exit code path. This looks like a real issue (and
   there seem to be other error-exit codepaths earlier in
   the function which leak this too.)

Would somebody like to have a look at these?

thanks
-- PMM

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 71c92f142a..cfa44b4153 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3312,26 +3312,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]) {
@@ -3384,7 +3388,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;
@@ -3400,15 +3404,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);