Le Saturday 08 Feb 2014 à 18:39:18 (+0100), Max Reitz a écrit :
> The fail and success paths of bdrv_file_open() may be further shortened
> by reusing code already existent in bdrv_open(). This includes
> bdrv_file_open() not taking the reference to options which allows the
> removal of QDECREF(options) in that function.
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> block.c | 68
> ++++++++++++++++++++++++++++++++---------------------------------
> 1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1fcd7e0..22e5a44 100644
> --- a/block.c
> +++ b/block.c
> @@ -948,13 +948,15 @@ free_and_fail:
> /*
> * Opens a file using a protocol (file, host_device, nbd, ...)
> *
> - * options is a QDict of options to pass to the block drivers, or NULL for an
> - * empty set of options. The reference to the QDict belongs to the block
> layer
> - * after the call (even on failure), so if the caller intends to reuse the
> - * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> + * 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)
> + QDict **options, int flags, Error **errp)
> {
> BlockDriver *drv;
> const char *drvname;
> @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const
> char *filename,
>
> /* 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));
> + filename = qdict_get_try_str(*options, "filename");
> + } else if (filename && !qdict_haskey(*options, "filename")) {
> + qdict_put(*options, "filename", qstring_from_str(filename));
> allow_protocol_prefix = true;
> } else {
> error_setg(errp, "Can't specify 'file' and 'filename' options at the
> "
> @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const
> char *filename,
> }
>
> /* Find the right block driver */
> - drvname = qdict_get_try_str(options, "driver");
> + 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");
> + qdict_del(*options, "driver");
> } else if (filename) {
> drv = bdrv_find_protocol(filename, allow_protocol_prefix);
> if (!drv) {
> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const
> char *filename,
>
> /* Parse the filename and open it */
> if (drv->bdrv_parse_filename && filename) {
> - drv->bdrv_parse_filename(filename, options, &local_err);
> + drv->bdrv_parse_filename(filename, *options, &local_err);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
> - qdict_del(options, "filename");
> + qdict_del(*options, "filename");
> + } else if (drv->bdrv_needs_filename && !filename) {
> + error_setg(errp, "The '%s' block driver requires a file name",
> + drv->format_name);
> + ret = -EINVAL;
> + goto fail;
> }
>
> if (!drv->bdrv_file_open) {
> - ret = bdrv_open(&bs, filename, NULL, options, flags, drv,
> &local_err);
> - options = NULL;
> + ret = bdrv_open(&bs, filename, NULL, *options, flags, drv,
> &local_err);
> + *options = NULL;
> } else {
> - ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> + ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
> }
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto fail;
> }
>
> - /* Check if any unknown options were used */
> - if (options && (qdict_size(options) != 0)) {
> - const QDictEntry *entry = qdict_first(options);
> - error_setg(errp, "Block protocol '%s' doesn't support the option
> '%s'",
> - drv->format_name, entry->key);
> - ret = -EINVAL;
> - goto fail;
> - }
> - QDECREF(options);
> -
> bs->growable = 1;
> return 0;
>
> fail:
> - QDECREF(options);
> return ret;
> }
>
> @@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char
> *filename,
>
> if (flags & BDRV_O_PROTOCOL) {
> assert(!drv);
> - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + ret = bdrv_file_open(bs, filename, &options, flags &
> ~BDRV_O_PROTOCOL,
> &local_err);
> - options = NULL;
> if (!ret) {
> - *pbs = bs;
> - return 0;
> + goto done;
> } else if (bs->drv) {
> goto close_and_fail;
> } else {
> @@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char
> *filename,
> }
> }
>
> +done:
> /* Check if any unknown options were used */
> - if (qdict_size(options) != 0) {
> + if (options && (qdict_size(options) != 0)) {
> const QDictEntry *entry = qdict_first(options);
> - error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> - "support the option '%s'", drv->format_name,
> bs->device_name,
> - entry->key);
> + if (flags & BDRV_O_PROTOCOL) {
> + error_setg(errp, "Block protocol '%s' doesn't support the option
> "
> + "'%s'", drv->format_name, entry->key);
> + } else {
> + error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> + "support the option '%s'", drv->format_name,
> + bs->device_name, entry->key);
> + }
>
> ret = -EINVAL;
> goto close_and_fail;
> --
> 1.8.5.4
>
>
Reviewed-by: Benoit Canet <ben...@irqsave.net>
