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

[Laszlo Ersek]

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.

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

blkdebug_open.patch
Description: Text document