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 <kw...@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
>
>
