Re: [Qemu-devel] [PATCH 2/9] block: Move bdrv_fill_options() call to bdrv_open()

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

The Wednesday 11 Jun 2014 à 16:04:56 (+0200), Kevin Wolf wrote :
> bs->options now contains the modified version of the options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c5707e8..effb3e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -1010,14 +1010,19 @@ free_and_fail:
>   * Fills in default options for opening images and converts the legacy
>   * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_fill_options(QDict **options, const char *filename,
> +static int bdrv_fill_options(QDict **options, const char *filename, int flags,
>                               Error **errp)
>  {
>      const char *drvname;
> +    bool protocol = flags & BDRV_O_PROTOCOL;
>      bool parse_filename = false;
>      Error *local_err = NULL;
>      BlockDriver *drv;
>  
> +    if (!protocol) {
> +        return 0;
> +    }
> +

Does this mean we are sure that we don't have image format with baroque parameters
in the filename ?
This does make sense.

>      /* Fetch the file name from the options QDict if necessary */
>      if (filename) {
>          if (filename && !qdict_haskey(*options, "filename")) {
> @@ -1082,20 +1087,15 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>   * returns. Then, the caller is responsible for freeing it. If it intends to
>   * reuse the QDict, QINCREF() should be called beforehand.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
> +                          Error **errp)
>  {
>      BlockDriver *drv;
> +    const char *filename;
>      const char *drvname;
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_fill_options(options, filename, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return ret;
> -    }
> -
>      filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_str(*options, "driver");
>  
> @@ -1443,12 +1443,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          filename = NULL;
>      }
>  
> +    ret = bdrv_fill_options(&options, filename, flags, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return ret;
Chance are that bs and option where created above.

goto fail ?

> +    }
> +
>      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,
> +        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
>                               &local_err);
>          if (!ret) {
>              drv = bs->drv;
> -- 
> 1.8.3.1
> 
>