On 29.01.21 06:26, Vladimir Sementsov-Ogievskiy wrote:
28.01.2021 21:38, Philippe Mathieu-Daudé wrote:
Hi Andrey,

On 1/26/21 3:19 PM, Max Reitz wrote:
From: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>

This patch completes the series with the COR-filter applied to
block-stream operations.

Adding the filter makes it possible in future implement discarding
copied regions in backing files during the block-stream job, to reduce
the disk overuse (we need control on permissions).

Also, the filter now is smart enough to do copy-on-read with specified
base, so we have benefit on guest reads even when doing block-stream of
the part of the backing chain.

Several iotests are slightly modified due to filter insertion.

Signed-off-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Message-Id: <20201216061703.70908-14-vsement...@virtuozzo.com>
Reviewed-by: Max Reitz <mre...@redhat.com>
Signed-off-by: Max Reitz <mre...@redhat.com>
---
  block/stream.c             | 105 ++++++++++++++++++++++---------------
  tests/qemu-iotests/030     |   8 +--
  tests/qemu-iotests/141.out |   2 +-
  tests/qemu-iotests/245     |  20 ++++---
  4 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/block/stream.c b/block/stream.c
...
@@ -228,7 +211,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
      bool bs_read_only;
      int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
      BlockDriverState *base_overlay;
+    BlockDriverState *cor_filter_bs = NULL;
      BlockDriverState *above_base;
+    QDict *opts;
      assert(!(base && bottom));
      assert(!(backing_file_str && bottom));
@@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,
          }
      }
-    if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
-        return;
-    }
-
      /* Make sure that the image is opened in read-write mode */
      bs_read_only = bdrv_is_read_only(bs);
      if (bs_read_only) {
-        if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
-            bs_read_only = false;
-            goto fail;
+        int ret;
+        /* Hold the chain during reopen */
+        if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
+            return;
+        }
+
+        ret = bdrv_reopen_set_read_only(bs, false, errp);
+
+        /* failure, or cor-filter will hold the chain */
+        bdrv_unfreeze_backing_chain(bs, above_base);
+
+        if (ret < 0) {
+            return;
          }
      }
-    /* Prevent concurrent jobs trying to modify the graph structure here, we -     * already have our own plans. Also don't allow resize as the image size is
-     * queried only at the job start and then cached. */
-    s = block_job_create(job_id, &stream_job_driver, NULL, bs,
-                         basic_flags | BLK_PERM_GRAPH_MOD,
+    opts = qdict_new();

Coverity reported (CID 1445793) that this resource could be leaked...

+
+    qdict_put_str(opts, "driver", "copy-on-read");
+    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+    /* Pass the base_overlay node name as 'bottom' to COR driver */
+    qdict_put_str(opts, "bottom", base_overlay->node_name);
+    if (filter_node_name) {
+        qdict_put_str(opts, "node-name", filter_node_name);
+    }
+
+    cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
+    if (!cor_filter_bs) {

... probably here.

Should we call g_free(opts) here?


Actually, not..

bdrv_insert_node() calls bdrv_open() which eats options even on failure.

I see, CID already marked false-positive?

Yes, I did that.

This isn’t the first time Coverity has reported a failed bdrv_open() call would leak the options QDict. Perhaps someone™ should look into why it likes to thinks that, but so far I haven’t been sufficiently bothered by it.

Max


Reply via email to