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

[Eric Blake]

On 06/11/2014 08:04 AM, 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 <address@hidden>
> ---
>  block.c | 116 
> ++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> + * 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)

The comment mentions filename/flags, but I only see filename as a
parameter.  Is this something changed later in the series?

>  {
> -    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 'filename &&' 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);

This assigns drv...

> +            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");
> +            return -EINVAL;
>          }
> -    } else {
> -        error_setg(errp, "Must specify either driver or file");
> -        drv = NULL;
>      }
>  
> +    drv = bdrv_find_format(drvname);

...and this does it again.  Should this second assignment be inside an
else{}?


> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> +                          QDict **options, int flags, Error **errp)

> +
> +    drv = bdrv_find_format(drvname);
> +    if (!drv) {
> +        error_setg(errp, "Unknown driver '%s'", drvname);
> +        ret = -ENOENT;
> +        goto fail;
> +    }

Isn't this error check redundant with the one already done in
bdrv_fill_options()?  You could probably just use assert(drv)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature