blkdebug: Don't leak bs->file on failure

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.

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.

Thanks,
Laszlo

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

On 02/08/14 10:55, Kevin Wolf wrote:
> Am 08.02.2014 um 10:46 hat Laszlo Ersek geschrieben:

>> 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.

Oh, right. If you actually care about the future, this is a good point :)

Thanks!
Laszlo