Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option.

[Kevin Wolf <kwolf@redhat.com>]

Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4f6b36a..61f5ba0 100644
> --- a/block.c
> +++ b/block.c
> @@ -873,6 +873,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      const char *drvname;
>      bool allow_protocol_prefix = false;
>      Error *local_err = NULL;
> +    const char *node_name = NULL;
>      int ret;
>  
>      /* NULL means an empty set of options */
> @@ -880,7 +881,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    bs = bdrv_new("", "");
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +    bs = bdrv_new("", node_name ? node_name : "");
> +    qdict_del(options, "node-name");
> +
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -980,6 +989,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      int back_flags, ret;
>      BlockDriver *back_drv = NULL;
>      Error *local_err = NULL;
> +    const char *node_name = NULL;
>  
>      if (bs->backing_hd != NULL) {
>          QDECREF(options);
> @@ -1002,7 +1012,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> -    bs->backing_hd = bdrv_new("", "");
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +    bs->backing_hd = bdrv_new("", node_name ? node_name : "");
> +    qdict_del(options, "node-name");
>  
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
> @@ -1046,6 +1063,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      BlockDriverState *file = NULL;
>      QDict *file_options = NULL;
>      const char *drvname;
> +    const char *node_name = NULL;
>      Error *local_err = NULL;
>  
>      /* NULL means an empty set of options */
> @@ -1053,6 +1071,22 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          options = qdict_new();
>      }
>  
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +
> +    if (node_name) {
> +        pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> +        if (node_name[0] != '\0') {
> +            QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> +        }
> +    }
> +
> +    qdict_del(options, "node-name");

We duplicate some code all over the place. In general you seem to be
trying to let the caller of bdrv_new() already figure out what the node
name is by parsing the options QDict; here however, you do it after
bdrv_new().

Can't we consolidate this and only ever set the node name in
bdrv_open_common(), so that the option is parsed only once, there is
only once place adding BDSes to the list, and there is only one place
checking for duplicates?

Kevin