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

[Benoît Canet]

Le Monday 09 Dec 2013 à 17:15:26 (+0100), Kevin Wolf a écrit :
> Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> > Signed-off-by: Benoit Canet <address@hidden>
> > ---
> >  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?

Nice idea it will make the patches much smaller and cleaner.

Best regards

Benoît

> 
> Kevin