Message ID | 1409119356-22742-5-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
The Wednesday 27 Aug 2014 à 14:02:36 (+0800), Fam Zheng wrote : > Not all the error paths release opts, fix it by moving it after the fail > label. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/qcow2.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index f9e045f..b4c4a6e 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -535,7 +535,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > unsigned int len, i; > int ret = 0; > QCowHeader header; > - QemuOpts *opts; > + QemuOpts *opts = NULL; > Error *local_err = NULL; > uint64_t ext_end; > uint64_t l1_vm_state_index; > @@ -932,7 +932,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(errp, "Unsupported value '%s' for qcow2 option " > "'overlap-check'. Allowed are either of the following: " > "none, constant, cached, all", opt_overlap_check); > - qemu_opts_del(opts); > ret = -EINVAL; > goto fail; > } > @@ -946,7 +945,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > overlap_check_template & (1 << i)) << i; > } > > - qemu_opts_del(opts); > > if (s->use_lazy_refcounts && s->qcow_version < 3) { > error_setg(errp, "Lazy refcounts require a qcow2 image with at least " > @@ -964,6 +962,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > return ret; The code is returning here in this case and the qemu_opts_del is hidden in the fail label. Do we need to qemu_opts_del on success just before return like it was done before ? > > fail: > + qemu_opts_del(opts); > g_free(s->unknown_header_fields); > cleanup_unknown_header_ext(bs); > qcow2_free_snapshots(bs); > -- > 2.1.0 > >
On 27.08.2014 08:02, Fam Zheng wrote: > Not all the error paths release opts, fix it by moving it after the fail > label. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/qcow2.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) This is pretty much the same patch as 'qcow2: Fix leak of QemuOpts in qcow2_open()' from my 'qapi/block-core: Add "new" qcow2 options' series. I guess it's up to the maintainer(s) which one gets pulled, I just want to point out that pulling one means leaving out the other. Max
On Wed, 08/27 16:40, Benoît Canet wrote: > The Wednesday 27 Aug 2014 à 14:02:36 (+0800), Fam Zheng wrote : > > Not all the error paths release opts, fix it by moving it after the fail > > label. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/qcow2.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index f9e045f..b4c4a6e 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -535,7 +535,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > unsigned int len, i; > > int ret = 0; > > QCowHeader header; > > - QemuOpts *opts; > > + QemuOpts *opts = NULL; > > Error *local_err = NULL; > > uint64_t ext_end; > > uint64_t l1_vm_state_index; > > @@ -932,7 +932,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > error_setg(errp, "Unsupported value '%s' for qcow2 option " > > "'overlap-check'. Allowed are either of the following: " > > "none, constant, cached, all", opt_overlap_check); > > - qemu_opts_del(opts); > > ret = -EINVAL; > > goto fail; > > } > > @@ -946,7 +945,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > overlap_check_template & (1 << i)) << i; > > } > > > > - qemu_opts_del(opts); > > > > if (s->use_lazy_refcounts && s->qcow_version < 3) { > > error_setg(errp, "Lazy refcounts require a qcow2 image with at least " > > @@ -964,6 +962,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > > > return ret; > The code is returning here in this case and the qemu_opts_del is hidden in the fail label. > Do we need to qemu_opts_del on success just before return like it was done before ? Yes, good catch. Thanks, Fam > > > > > fail: > > + qemu_opts_del(opts); > > g_free(s->unknown_header_fields); > > cleanup_unknown_header_ext(bs); > > qcow2_free_snapshots(bs); > > -- > > 2.1.0 > > > >
On Wed, 08/27 19:40, Max Reitz wrote: > On 27.08.2014 08:02, Fam Zheng wrote: > >Not all the error paths release opts, fix it by moving it after the fail > >label. > > > >Signed-off-by: Fam Zheng <famz@redhat.com> > >--- > > block/qcow2.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > This is pretty much the same patch as 'qcow2: Fix leak of QemuOpts in > qcow2_open()' from my 'qapi/block-core: Add "new" qcow2 options' series. I > guess it's up to the maintainer(s) which one gets pulled, I just want to > point out that pulling one means leaving out the other. Since I need to respin this series, I will drop this one and leave it for your series. Thanks for pointing out. :) Fam
diff --git a/block/qcow2.c b/block/qcow2.c index f9e045f..b4c4a6e 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -535,7 +535,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, unsigned int len, i; int ret = 0; QCowHeader header; - QemuOpts *opts; + QemuOpts *opts = NULL; Error *local_err = NULL; uint64_t ext_end; uint64_t l1_vm_state_index; @@ -932,7 +932,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, error_setg(errp, "Unsupported value '%s' for qcow2 option " "'overlap-check'. Allowed are either of the following: " "none, constant, cached, all", opt_overlap_check); - qemu_opts_del(opts); ret = -EINVAL; goto fail; } @@ -946,7 +945,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, overlap_check_template & (1 << i)) << i; } - qemu_opts_del(opts); if (s->use_lazy_refcounts && s->qcow_version < 3) { error_setg(errp, "Lazy refcounts require a qcow2 image with at least " @@ -964,6 +962,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, return ret; fail: + qemu_opts_del(opts); g_free(s->unknown_header_fields); cleanup_unknown_header_ext(bs); qcow2_free_snapshots(bs);
Not all the error paths release opts, fix it by moving it after the fail label. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/qcow2.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)