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

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

[Kevin Wolf]

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 56c4cd0..8eb0db0 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -396,14 +396,14 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-        goto fail;
+        goto out;
     }
 
     /* Read rules from config file or command line options */
     config = qemu_opt_get(opts, "config");
     ret = read_config(s, config, options, errp);
     if (ret) {
-        goto fail;
+        goto out;
     }
 
     /* Set initial state */
@@ -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 out;
     }
 
     /* Set request alignment */
@@ -424,11 +424,15 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     } else {
         error_setg(errp, "Invalid alignment");
         ret = -EINVAL;
-        goto fail;
+        goto fail_unref;
     }
 
     ret = 0;
-fail:
+    goto out;
+
+fail_unref:
+    bdrv_unref(bs->file);
+out:
     qemu_opts_del(opts);
     return ret;
 }
-- 
1.8.1.4

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

[Laszlo Ersek]

On 02/08/14 11:00, Kevin Wolf wrote:
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 56c4cd0..8eb0db0 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -396,14 +396,14 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>          ret = -EINVAL;
> -        goto fail;
> +        goto out;
>      }
>  
>      /* Read rules from config file or command line options */
>      config = qemu_opt_get(opts, "config");
>      ret = read_config(s, config, options, errp);
>      if (ret) {
> -        goto fail;
> +        goto out;
>      }
>  
>      /* Set initial state */
> @@ -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 out;
>      }
>  
>      /* Set request alignment */
> @@ -424,11 +424,15 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      } else {
>          error_setg(errp, "Invalid alignment");
>          ret = -EINVAL;
> -        goto fail;
> +        goto fail_unref;
>      }
>  
>      ret = 0;
> -fail:
> +    goto out;
> +
> +fail_unref:
> +    bdrv_unref(bs->file);
> +out:
>      qemu_opts_del(opts);
>      return ret;
>  }
> 

I think this is correct, but the standalone "goto out" looks awful :)

In such cases, when I have to combine unconditional teardown with
release-on-error-only, I apply a "nested ifs" pattern, where the
release-on-error-only bits (on the way out) depend on the function exit
status. But "nested ifs" is foreign for qemu, so I think these goto's
actually match the qemu style.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo