Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

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

The Wednesday 11 Jun 2014 à 16:04:55 (+0200), Kevin Wolf wrote :
> The idea of bdrv_fill_options() is to convert every parameter for
> opening images, in particular the filename and flags, to entries in the
> options QDict.
> 
> This patch starts with moving the filename parsing and driver probing
> part from bdrv_file_open() to the new function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 17f763d..c5707e8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1007,77 +1007,107 @@ free_and_fail:
>  }
>  
>  /*
> - * Opens a file using a protocol (file, host_device, nbd, ...)
> - *
> - * options is an indirect pointer to a QDict of options to pass to the block
> - * drivers, or pointer to NULL for an empty set of options. If this function
> - * takes ownership of the QDict reference, it will set *options to NULL;
> - * otherwise, it will contain unused/unrecognized options after this function
> - * returns. Then, the caller is responsible for freeing it. If it intends to
> - * reuse the QDict, QINCREF() should be called beforehand.
> + * Fills in default options for opening images and converts the legacy
> + * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_fill_options(QDict **options, const char *filename,
> +                             Error **errp)
>  {
> -    BlockDriver *drv;
>      const char *drvname;
>      bool parse_filename = false;
>      Error *local_err = NULL;
> -    int ret;
> +    BlockDriver *drv;
>  
>      /* Fetch the file name from the options QDict if necessary */
> -    if (!filename) {
> -        filename = qdict_get_try_str(*options, "filename");
> -    } else if (filename && !qdict_haskey(*options, "filename")) {
> -        qdict_put(*options, "filename", qstring_from_str(filename));
> -        parse_filename = true;
> -    } else {
> -        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> -                   "same time");
> -        ret = -EINVAL;
> -        goto fail;
> +    if (filename) {
> +        if (filename && !qdict_haskey(*options, "filename")) {

The inner filename testing is redundant.

> +            qdict_put(*options, "filename", qstring_from_str(filename));
> +            parse_filename = true;
> +        } else {
> +            error_setg(errp, "Can't specify 'file' and 'filename' options at "
> +                             "the same time");
> +            return -EINVAL;
> +        }
>      }
>  
>      /* Find the right block driver */
> +    filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_try_str(*options, "driver");
> -    if (drvname) {
> -        drv = bdrv_find_format(drvname);
> -        if (!drv) {
> -            error_setg(errp, "Unknown driver '%s'", drvname);
> -        }
> -        qdict_del(*options, "driver");
> -    } else if (filename) {
> -        drv = bdrv_find_protocol(filename, parse_filename);
> -        if (!drv) {
> -            error_setg(errp, "Unknown protocol");
> +
> +    if (!drvname) {
> +        if (filename) {
> +            drv = bdrv_find_protocol(filename, parse_filename);
> +            if (!drv) {
> +                error_setg(errp, "Unknown protocol");
> +                return -EINVAL;
> +            }
> +
> +            drvname = drv->format_name;
> +            qdict_put(*options, "driver", qstring_from_str(drvname));
> +        } else {
> +            error_setg(errp, "Must specify either driver or file");

Isn't it "Must specify either driver or _filename_ ?

> +            return -EINVAL;
>          }
> -    } else {
> -        error_setg(errp, "Must specify either driver or file");
> -        drv = NULL;
>      }
>  
> +    drv = bdrv_find_format(drvname);
>      if (!drv) {
> -        /* errp has been set already */
> -        ret = -ENOENT;
> -        goto fail;
> +        error_setg(errp, "Unknown driver '%s'", drvname);
> +        return -ENOENT;
>      }
>  
> -    /* Parse the filename and open it */
> +    /* Driver-specific filename parsing */
>      if (drv->bdrv_parse_filename && parse_filename) {
>          drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            ret = -EINVAL;
> -            goto fail;
> +            return -EINVAL;
>          }
>  
>          if (!drv->bdrv_needs_filename) {
>              qdict_del(*options, "filename");
> -        } else {
> -            filename = qdict_get_str(*options, "filename");
>          }
>      }
>  
> +    return 0;
> +}
> +
> +/*
> + * Opens a file using a protocol (file, host_device, nbd, ...)
> + *
> + * options is an indirect pointer to a QDict of options to pass to the block
> + * drivers, or pointer to NULL for an empty set of options. If this function
> + * takes ownership of the QDict reference, it will set *options to NULL;
> + * otherwise, it will contain unused/unrecognized options after this function
> + * 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)
> +{
> +    BlockDriver *drv;
> +    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;
Why not goto fail for consistency with the rest of the function ?

> +    }
> +
> +    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) {

Don't we need:
           qdict_del(*options, "filename");
Here so bdrv_open don't see an extra filename option ?

>          ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
>          *options = NULL;
> -- 
> 1.8.3.1
> 
>