Re: [Qemu-devel] [PATCH 5/9] block: Use common driver selection code for bdrv_open_file()

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

The Wednesday 11 Jun 2014 à 16:04:59 (+0200), Kevin Wolf wrote :
> This moves the bdrv_open_file() call a bit down so that it can use the
> bdrv_open() code that selects the right block driver.
> 
> The code between the old and the new call site is either common code
> (the error message for an unknown driver has been unified now) or
> doesn't run with cleared BDRV_O_PROTOCOL (added an if block in one
> place, whereas the right path was already asserted in another place)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 71 ++++++++++++++++++++++++++++-------------------------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 011cfc4..2aa0a56 100644
> --- a/block.c
> +++ b/block.c
> @@ -1136,25 +1136,14 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>   * 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, QDict **options, int flags,
> -                          Error **errp)
> +static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
> +                          QDict **options, int flags, Error **errp)
>  {
> -    BlockDriver *drv;
>      const char *filename;
> -    const char *drvname;
>      Error *local_err = NULL;
>      int ret;
>  
>      filename = qdict_get_try_str(*options, "filename");
> -    drvname = qdict_get_str(*options, "driver");
> -
> -    drv = bdrv_find_format(drvname);
> -    if (!drv) {
> -        error_setg(errp, "Unknown driver '%s'", drvname);
> -        ret = -ENOENT;
> -        goto fail;
> -    }
> -    qdict_del(*options, "driver");
>  
>      /* Open the file */
>      if (!drv->bdrv_file_open) {
> @@ -1460,35 +1449,23 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> -    if (flags & BDRV_O_PROTOCOL) {
maybe you could use a bool protocol = flags & BDRV_O_PROTOCOL at the begining of
the function.
> -        assert(!drv);
> -        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
> -                             &local_err);
> -        if (!ret) {
> -            drv = bs->drv;
> -            goto done;
> -        } else if (bs->drv) {
> -            goto close_and_fail;
> -        } else {
> -            goto fail;
> -        }
> -    }
> -
>      /* Open image file without format layer */
> -    if (flags & BDRV_O_RDWR) {
> -        flags |= BDRV_O_ALLOW_RDWR;
> -    }
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        snapshot_flags = bdrv_temp_snapshot_flags(flags);
> -        flags = bdrv_backing_flags(flags);
> -    }
> +    if ((flags & BDRV_O_PROTOCOL) == 0) {
> +        if (flags & BDRV_O_RDWR) {
> +            flags |= BDRV_O_ALLOW_RDWR;
> +        }
> +        if (flags & BDRV_O_SNAPSHOT) {
> +            snapshot_flags = bdrv_temp_snapshot_flags(flags);
> +            flags = bdrv_backing_flags(flags);
> +        }
>  
> -    assert(file == NULL);
> -    ret = bdrv_open_image(&file, filename, options, "file",
> -                          bdrv_inherited_flags(flags),
> -                          true, &local_err);
> -    if (ret < 0) {
> -        goto fail;
> +        assert(file == NULL);
> +        ret = bdrv_open_image(&file, filename, options, "file",
> +                              bdrv_inherited_flags(flags),
> +                              true, &local_err);
> +        if (ret < 0) {
> +            goto fail;
> +        }
>      }
>  
>      /* Find the right image format driver */
> @@ -1500,7 +1477,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          drv = bdrv_find_format(drvname);
>          qdict_del(options, "driver");
>          if (!drv) {
> -            error_setg(errp, "Invalid driver: '%s'", drvname);
> +            error_setg(errp, "Unknown driver: '%s'", drvname);
>              ret = -EINVAL;
>              goto fail;
>          }
> @@ -1516,6 +1493,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      /* Open the image */
> +    if (flags & BDRV_O_PROTOCOL) {
> +        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
> +                             &local_err);
> +        if (!ret) {
> +            goto done;
> +        } else if (bs->drv) {
> +            goto close_and_fail;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
>      if (ret < 0) {
>          goto fail;
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>