[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] blkdebug: Don't leak bs->file on failure
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v2] blkdebug: Don't leak bs->file on failure |
Date: |
Sat, 08 Feb 2014 11:52:13 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131118 Thunderbird/17.0.11 |
On 02/08/14 11:00, Kevin Wolf wrote:
> Reported-by: Laszlo Ersek <address@hidden>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> 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 <address@hidden>
Thanks!
Laszlo