[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-na
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option. |
Date: |
Wed, 11 Dec 2013 15:42:56 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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