Re: [Qemu-devel] [PATCH] blkdebug: Don't leak bs->file on failure

[Kevin Wolf <kwolf@redhat.com>]

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 <kwolf@redhat.com>
> > ---
> >  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