Re: [Qemu-devel] [PATCH 18/34] qcow2: Fix memory leak in qcow2_update_options() error path

[Kevin Wolf <kwolf@redhat.com>]

Am 13.05.2015 um 13:52 hat Max Reitz geschrieben:
> On 08.05.2015 19:21, Kevin Wolf wrote:
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block/qcow2.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index abe22f3..84d6e0f 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -546,8 +546,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> >      const char *opt_overlap_check, *opt_overlap_check_template;
> >      int overlap_check_template = 0;
> >      uint64_t l2_cache_size, refcount_cache_size;
> >-    Qcow2Cache* l2_table_cache;
> >-    Qcow2Cache* refcount_block_cache;
> >+    Qcow2Cache* l2_table_cache = NULL;
> >+    Qcow2Cache* refcount_block_cache = NULL;
> >      bool use_lazy_refcounts;
> >      int i;
> >      Error *local_err = NULL;
> >@@ -670,6 +670,14 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> >      ret = 0;
> >  fail:
> >+    if (ret < 0) {
> >+        if (l2_table_cache) {
> >+            qcow2_cache_destroy(bs, l2_table_cache);
> >+        }
> >+        if (refcount_block_cache) {
> >+            qcow2_cache_destroy(bs, refcount_block_cache);
> >+        }
> >+    }
> >      qemu_opts_del(opts);
> >      opts = NULL;
> 
> Why don't you squash this into patch 17 (I guess there is a reason,
> but I fail to see it)?

It's a preexisting bug and its fix is unrelated to any of the
refactoring or preparation for reopen. So I think it deserves its own
commit, and if it doesn't, it's not clear to me why it should belong to
patch 17 of all patches.

> Also, I think it might make sense to either set
> {l2_table,refcount_block}_cache to NULL once they have been moved to
> s->{l2_table,refcount_block}_cache, or, even better, exchange them
> with their respective field in *s. Thus, you could drop the "if (ret
> < 0)", I think I'd find the code easier to read, and with the
> transation, we'd even be safe if the options had been set before and
> are now to be updated.

The if (ret < 0) disappears shortly after this patch when this is moved
into the abort part of a transaction.

Kevin