Re: [Qemu-devel] [PATCH 4/9] block: Always pass driver name through options QDict

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

The Wednesday 11 Jun 2014 à 16:04:58 (+0200), Kevin Wolf wrote :
> The "driver" entry in the options QDict is now only missing if we're
> opening an image with format probing.
> 
> We also catch cases now where both the drv argument and a "driver"
> option is specified, e.g. by specifying -drive format=qcow2,driver=raw
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    | 76 ++++++++++++++++++++++++----------------------
>  tests/qemu-iotests/051     |  1 +
>  tests/qemu-iotests/051.out |  5 ++-
>  3 files changed, 45 insertions(+), 37 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b9c2b41..011cfc4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1038,14 +1038,13 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
>   * filename/flags pair to option QDict entries.
>   */
>  static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
> -                             Error **errp)
> +                             BlockDriver *drv, Error **errp)
>  {
>      const char *filename = *pfilename;
>      const char *drvname;
>      bool protocol = flags & BDRV_O_PROTOCOL;
>      bool parse_filename = false;
>      Error *local_err = NULL;
> -    BlockDriver *drv;
>  
>      /* Parse json: pseudo-protocol */
>      if (filename && g_str_has_prefix(filename, "json:")) {
> @@ -1062,12 +1061,8 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>          *pfilename = filename = NULL;
>      }
>  
> -    if (!protocol) {
> -        return 0;
> -    }
> -
>      /* Fetch the file name from the options QDict if necessary */
> -    if (filename) {
> +    if (protocol && filename) {
>          if (filename && !qdict_haskey(*options, "filename")) {
>              qdict_put(*options, "filename", qstring_from_str(filename));
>              parse_filename = true;
> @@ -1082,30 +1077,41 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>      filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_try_str(*options, "driver");
>  
> -    if (!drvname) {
> -        if (filename) {
> -            drv = bdrv_find_protocol(filename, parse_filename);
> -            if (!drv) {
> -                error_setg(errp, "Unknown protocol");
> +    if (drv) {
> +        if (drvname) {
> +            error_setg(errp, "Driver specified twice");
> +            return -EINVAL;
> +        }
> +        drvname = drv->format_name;
> +        qdict_put(*options, "driver", qstring_from_str(drvname));
> +    } else {
> +        if (!drvname && protocol) {
> +            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");
>                  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 if (drvname) {
> +            drv = bdrv_find_format(drvname);
> +            if (!drv) {
> +                error_setg(errp, "Unknown driver '%s'", drvname);
> +                return -ENOENT;
> +            }
>          }
>      }
>  
> -    drv = bdrv_find_format(drvname);
> -    if (!drv) {
> -        error_setg(errp, "Unknown driver '%s'", drvname);
> -        return -ENOENT;
> -    }
> +    assert(drv || !protocol);
>  
>      /* Driver-specific filename parsing */
> -    if (drv->bdrv_parse_filename && parse_filename) {
> +    if (drv && drv->bdrv_parse_filename && parse_filename) {
>          drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> @@ -1445,7 +1451,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
> +    ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return ret;
> @@ -1486,7 +1492,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      /* Find the right image format driver */
> +    drv = NULL;
>      drvname = qdict_get_try_str(options, "driver");
> +    assert(drvname || !(flags & BDRV_O_PROTOCOL));
> +
>      if (drvname) {
>          drv = bdrv_find_format(drvname);
>          qdict_del(options, "driver");
> @@ -1495,19 +1504,14 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>              ret = -EINVAL;
>              goto fail;
>          }
> -    }
> -
> -    if (!drv) {
> -        if (file) {
> -            ret = find_image_format(file, filename, &drv, &local_err);
> -        } else {
> -            error_setg(errp, "Must specify either driver or file");
> -            ret = -EINVAL;
> +    } else if (file) {
> +        ret = find_image_format(file, filename, &drv, &local_err);
> +        if (ret < 0) {
>              goto fail;
>          }
> -    }
> -
> -    if (!drv) {
> +    } else {
> +        error_setg(errp, "Must specify either driver or file");
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index c4af131..30a712f 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -92,6 +92,7 @@ echo
>  
>  run_qemu -drive file="$TEST_IMG",format=foo
>  run_qemu -drive file="$TEST_IMG",driver=foo
> +run_qemu -drive file="$TEST_IMG",driver=raw,format=qcow2
>  
>  echo
>  echo === Overriding backing file ===
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index ad60107..37cb049 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -38,7 +38,10 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=foo
>  QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=foo: 'foo' invalid format
>  
>  Testing: -drive file=TEST_DIR/t.qcow2,driver=foo
> -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Invalid driver: 'foo'
> +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Unknown driver 'foo'
> +
> +Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2
> +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2: could not open disk image TEST_DIR/t.qcow2: Driver specified twice
>  
>  
>  === Overriding backing file ===
> -- 
> 1.8.3.1
> 
> 
That's faily huge patch. I didn't understood everything.