[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] blkdebug: Don't leak bs->file on failure
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] blkdebug: Don't leak bs->file on failure |
Date: |
Sat, 8 Feb 2014 10:55:37 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 08.02.2014 um 10:46 hat Laszlo Ersek geschrieben:
> comments below
>
> On 02/08/14 09:54, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> > block/blkdebug.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/blkdebug.c b/block/blkdebug.c
> > index 56c4cd0..519b483 100644
> > --- a/block/blkdebug.c
> > +++ b/block/blkdebug.c
> > @@ -414,7 +414,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > flags, true, false, &local_err);
> > if (ret < 0) {
> > error_propagate(errp, local_err);
> > - goto fail;
> > + goto fail_unref;
> > }
>
> This block appears to run when the bdrv_open_image(&bs->file, ...) call
> fails. If we can't open the backing file, are you sure we need to unref
> it under fail_unref? I thought that this block needed no update (one
> only needs to extend the error handling when allocation of a new
> resource is successful), but I may have been wrong of course. (Generally
> in qemu it's always anyone's best guess who is responsible for releasing
> stuff on the error path, caller or callee.)
>
> >
> > /* Set request alignment */
> > @@ -424,10 +424,13 @@ static int blkdebug_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > } else {
> > error_setg(errp, "Invalid alignment");
> > ret = -EINVAL;
> > - goto fail;
> > + goto fail_unref;
> > }
>
> I do agree with this.
>
> >
> > - ret = 0;
> > + return 0;
> > +
> > +fail_unref:
> > + bdrv_unref(bs->file);
> > fail:
> > qemu_opts_del(opts);
> > return ret;
> >
>
> I believe this change causes us to leak "opts" on success.
>
> ... Unless we should have hung on to it to begin with, of course, in
> which case this is a silent bugfix. But I don't believe so; I think we
> only need "opts" temporarily, to absorb options from the QDict, and for
> ease of parsing into local variables.
As always, you're right with all you're saying. Sorry for being sloppy.
> How about the attached patch instead? Releasing stuff in specific error
> handler blocks quickly becomes intractable if there are many resources
> to allocate in succession, but in this case I think we can easily get
> away with it.
I'd prefer to avoid this because it's too easy to miss it when you add
new code to the function. Let me send a v2 that keeps fail_unref.
Kevin