Patchwork [2/2] block: Allow the user to define "node-name" option.

login
register
mail settings
Submitter Benoît Canet
Date Nov. 7, 2013, 3:01 p.m.
Message ID <1383836503-25447-3-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/289380/
State New
Headers show

Comments

Benoît Canet - Nov. 7, 2013, 3:01 p.m.
As node-name is a separate name space as device-name we can enable it's
definition right now: nobody will use it so no harm involved.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Eric Blake - Nov. 7, 2013, 8:31 p.m.
On 11/07/2013 08:01 AM, Benoît Canet wrote:
> As node-name is a separate name space as device-name we can enable it's

s/space as/space from/
s/it's/its/

> definition right now: nobody will use it so no harm involved.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Shouldn't blockdev-add have a QMP counterpart for setting node name
during device hotplug?  Also, is there a QMP command for inspecting node
names yet?  This feels like a write-only interface if we don't have more
usage of it in place.

Don't get me wrong - I think we definitely want this, but in the context
of a bigger series rather than by itself.

> 
> diff --git a/block.c b/block.c
> index 230e71a..132981f 100644
> --- a/block.c
> +++ b/block.c
> @@ -885,7 +885,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    bs = bdrv_new("", NULL);
> +    bs = bdrv_new("", qdict_get_try_str(options, "node-name"));
> +    qdict_del(options, "node-name");
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -1007,7 +1008,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> -    bs->backing_hd = bdrv_new("", NULL);
> +    bs->backing_hd = bdrv_new("", qdict_get_try_str(options, "node-name"));
> +    qdict_del(options, "node-name");
>  
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
>
Kevin Wolf - Nov. 8, 2013, 9:55 a.m.
Am 07.11.2013 um 16:01 hat Benoît Canet geschrieben:
> As node-name is a separate name space as device-name we can enable it's
> definition right now: nobody will use it so no harm involved.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>

As Eric already said, we need to update the QAPI schema for the new
field.

We already have an 'id' field for each BlockDriverState, which is
currently used as the device_name on the top level, and forbidden on
other devices. What is our long-term plan with this?

Should 'id' be for every node that must get a BlockBackend on top, and
'node-name' just for any node? We're mixing BDS and BB configuration
here. It may be okay to do that, especially as long as BBs don't have
more than just device_name to be configured, but we should at least be
aware that we're doing it.

Kevin

Patch

diff --git a/block.c b/block.c
index 230e71a..132981f 100644
--- a/block.c
+++ b/block.c
@@ -885,7 +885,8 @@  int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    bs = bdrv_new("", NULL);
+    bs = bdrv_new("", qdict_get_try_str(options, "node-name"));
+    qdict_del(options, "node-name");
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1007,7 +1008,8 @@  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
                                        sizeof(backing_filename));
     }
 
-    bs->backing_hd = bdrv_new("", NULL);
+    bs->backing_hd = bdrv_new("", qdict_get_try_str(options, "node-name"));
+    qdict_del(options, "node-name");
 
     if (bs->backing_format[0] != '\0') {
         back_drv = bdrv_find_format(bs->backing_format);