Re: [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only

["Benoît Canet" <benoit.canet@irqsave.net>]

Le Saturday 08 Feb 2014 à 18:39:17 (+0100), Max Reitz a écrit :
> The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
> similarities, thus it is possible to reuse the one from bdrv_open() and
> shorten the one in bdrv_file_open() accordingly.
> 
> Also, setting bs->options in bdrv_file_open() is not necessary if it is
> already done in bdrv_open().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c57ad2c..1fcd7e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    /* NULL means an empty set of options */
> -    if (options == NULL) {
> -        options = qdict_new();
> -    }
> -
> -    bs->options = options;
> -    options = qdict_clone_shallow(options);
> -
>      /* Fetch the file name from the options QDict if necessary */
>      if (!filename) {
>          filename = qdict_get_try_str(options, "filename");
> @@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>  fail:
>      QDECREF(options);
> -    if (!bs->drv) {
> -        QDECREF(bs->options);
> -    }
>      return ret;
>  }
>  
> @@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          bs = bdrv_new("");
>      }
>  
> -    if (flags & BDRV_O_PROTOCOL) {
> -        assert(!drv);
> -        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> -                             errp);
> -        if (ret && !*pbs) {
> -            bdrv_unref(bs);
> -        } else if (!ret) {
> -            *pbs = bs;
> -        }
> -        return ret;
> -    }
> -
>      /* NULL means an empty set of options */
>      if (options == NULL) {
>          options = qdict_new();
> @@ -1263,6 +1240,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> +    if (flags & BDRV_O_PROTOCOL) {
> +        assert(!drv);
> +        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> +                             &local_err);
> +        options = NULL;
> +        if (!ret) {
> +            *pbs = bs;
> +            return 0;
> +        } else if (bs->drv) {
> +            goto close_and_fail;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      /* For snapshot=on, create a temporary qcow2 overlay */
>      if (flags & BDRV_O_SNAPSHOT) {
>          BlockDriverState *bs1;
> -- 
> 1.8.5.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>