diff mbox series

[RFC,15/22] block/export: Move device to BlockExportOptions

Message ID 20200813162935.210070-16-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
Every block export needs a block node to export, so move the 'device'
option from BlockExportOptionsNbd to BlockExportOptions.

To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
to be wrapped by a new type NbdServerAddOptions that adds 'device' back
because nbd-server-add doesn't use the BlockExportOptions base type.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-export.json         | 27 +++++++++++++++++++++------
 block/export/export.c          | 26 ++++++++++++++++++++------
 block/monitor/block-hmp-cmds.c |  6 +++---
 blockdev-nbd.c                 |  4 ++--
 qemu-nbd.c                     |  2 +-
 5 files changed, 47 insertions(+), 18 deletions(-)

Comments

Max Reitz Aug. 17, 2020, 3:13 p.m. UTC | #1
On 13.08.20 18:29, Kevin Wolf wrote:
> Every block export needs a block node to export, so move the 'device'
> option from BlockExportOptionsNbd to BlockExportOptions.
> 
> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
> to be wrapped by a new type NbdServerAddOptions that adds 'device' back
> because nbd-server-add doesn't use the BlockExportOptions base type.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-export.json         | 27 +++++++++++++++++++++------
>  block/export/export.c          | 26 ++++++++++++++++++++------
>  block/monitor/block-hmp-cmds.c |  6 +++---
>  blockdev-nbd.c                 |  4 ++--
>  qemu-nbd.c                     |  2 +-
>  5 files changed, 47 insertions(+), 18 deletions(-)

(Code diff looks good, just a question on the interface:)

> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 4ce163411f..d68f3bf87e 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json

[...]

> @@ -167,6 +179,8 @@
>  # Describes a block export, i.e. how single node should be exported on an
>  # external interface.
>  #
> +# @device: The device name or node name of the node to be exported
> +#

Wouldn’t it be better to restrict ourselves to a node name here?
(Bluntly ignoring the fact that doing so would make this patch an
incompatible change, which would require some reordering in this series,
unless we decide to just ignore that intra-series incompatibility.)

OTOH...  What does “better” mean.  It won’t hurt anyone to keep this as
@device.  It’s just that I feel like if we had no legacy burden and did
this all from scratch, we would just make it @node-name.

Did you deliberately decide against @node-name?

Max
Kevin Wolf Aug. 17, 2020, 3:27 p.m. UTC | #2
Am 17.08.2020 um 17:13 hat Max Reitz geschrieben:
> On 13.08.20 18:29, Kevin Wolf wrote:
> > Every block export needs a block node to export, so move the 'device'
> > option from BlockExportOptionsNbd to BlockExportOptions.
> > 
> > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
> > to be wrapped by a new type NbdServerAddOptions that adds 'device' back
> > because nbd-server-add doesn't use the BlockExportOptions base type.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-export.json         | 27 +++++++++++++++++++++------
> >  block/export/export.c          | 26 ++++++++++++++++++++------
> >  block/monitor/block-hmp-cmds.c |  6 +++---
> >  blockdev-nbd.c                 |  4 ++--
> >  qemu-nbd.c                     |  2 +-
> >  5 files changed, 47 insertions(+), 18 deletions(-)
> 
> (Code diff looks good, just a question on the interface:)
> 
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 4ce163411f..d68f3bf87e 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> 
> [...]
> 
> > @@ -167,6 +179,8 @@
> >  # Describes a block export, i.e. how single node should be exported on an
> >  # external interface.
> >  #
> > +# @device: The device name or node name of the node to be exported
> > +#
> 
> Wouldn’t it be better to restrict ourselves to a node name here?
> (Bluntly ignoring the fact that doing so would make this patch an
> incompatible change, which would require some reordering in this series,
> unless we decide to just ignore that intra-series incompatibility.)

We already have intra-series incompatibility, so I wouldn't mind that.

> OTOH...  What does “better” mean.  It won’t hurt anyone to keep this as
> @device.  It’s just that I feel like if we had no legacy burden and did
> this all from scratch, we would just make it @node-name.
> 
> Did you deliberately decide against @node-name?

At first I thought I could still share code between nbd-server-add and
block-export-add, but that's not the case. Then I guess the only other
reason I have is consistency with other QMP commands. I won't pretend
it's a strong one.

Kevin
Max Reitz Aug. 17, 2020, 3:49 p.m. UTC | #3
On 17.08.20 17:27, Kevin Wolf wrote:
> Am 17.08.2020 um 17:13 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> Every block export needs a block node to export, so move the 'device'
>>> option from BlockExportOptionsNbd to BlockExportOptions.
>>>
>>> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
>>> to be wrapped by a new type NbdServerAddOptions that adds 'device' back
>>> because nbd-server-add doesn't use the BlockExportOptions base type.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qapi/block-export.json         | 27 +++++++++++++++++++++------
>>>  block/export/export.c          | 26 ++++++++++++++++++++------
>>>  block/monitor/block-hmp-cmds.c |  6 +++---
>>>  blockdev-nbd.c                 |  4 ++--
>>>  qemu-nbd.c                     |  2 +-
>>>  5 files changed, 47 insertions(+), 18 deletions(-)
>>
>> (Code diff looks good, just a question on the interface:)
>>
>>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>>> index 4ce163411f..d68f3bf87e 100644
>>> --- a/qapi/block-export.json
>>> +++ b/qapi/block-export.json
>>
>> [...]
>>
>>> @@ -167,6 +179,8 @@
>>>  # Describes a block export, i.e. how single node should be exported on an
>>>  # external interface.
>>>  #
>>> +# @device: The device name or node name of the node to be exported
>>> +#
>>
>> Wouldn’t it be better to restrict ourselves to a node name here?
>> (Bluntly ignoring the fact that doing so would make this patch an
>> incompatible change, which would require some reordering in this series,
>> unless we decide to just ignore that intra-series incompatibility.)
> 
> We already have intra-series incompatibility, so I wouldn't mind that.
> 
>> OTOH...  What does “better” mean.  It won’t hurt anyone to keep this as
>> @device.  It’s just that I feel like if we had no legacy burden and did
>> this all from scratch, we would just make it @node-name.
>>
>> Did you deliberately decide against @node-name?
> 
> At first I thought I could still share code between nbd-server-add and
> block-export-add, but that's not the case. Then I guess the only other
> reason I have is consistency with other QMP commands. I won't pretend
> it's a strong one.

If “using @node-name would be more natural” doesn’t resonate with you,
then I suppose we should just leave it as @device.  It isn’t harder to
handle for qemu, and maybe it makes transitioning easier for some users
(although I can’t quite imagine how).

Max
Eric Blake Aug. 19, 2020, 9:13 p.m. UTC | #4
On 8/13/20 11:29 AM, Kevin Wolf wrote:
> Every block export needs a block node to export, so move the 'device'
> option from BlockExportOptionsNbd to BlockExportOptions.
> 
> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
> to be wrapped by a new type NbdServerAddOptions that adds 'device' back
> because nbd-server-add doesn't use the BlockExportOptions base type.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/qapi/block-export.json
> @@ -62,9 +62,8 @@
>   ##
>   # @BlockExportOptionsNbd:
>   #
> -# An NBD block export.
> -#
> -# @device: The device name or node name of the node to be exported
> +# An NBD block export (options shared between nbd-server-add and the NBD branch
> +# of block-export-add).
>   #
>   # @name: Export name. If unspecified, the @device parameter is used as the
>   #        export name. (Since 2.12)
> @@ -82,8 +81,21 @@
>   # Since: 5.0
>   ##
>   { 'struct': 'BlockExportOptionsNbd',
> -  'data': {'device': 'str', '*name': 'str', '*description': 'str',
> -           '*writable': 'bool', '*bitmap': 'str' } }
> +  'data': { '*name': 'str', '*description': 'str',
> +            '*writable': 'bool', '*bitmap': 'str' } }
> +
> +##
> +# @NbdServerAddOptions:
> +#
> +# An NBD block export.
> +#
> +# @device: The device name or node name of the node to be exported
> +#
> +# Since: 5.0

5.2, now?

> +##
> +{ 'struct': 'NbdServerAddOptions',
> +  'base': 'BlockExportOptionsNbd',
> +  'data': { 'device': 'str' } }

I understand why nbd sticks with device that can name device or node, but...

>   
>   ##
>   # @nbd-server-add:
> @@ -96,7 +108,7 @@
>   # Since: 1.3.0
>   ##
>   { 'command': 'nbd-server-add',
> -  'data': 'BlockExportOptionsNbd', 'boxed': true }
> +  'data': 'NbdServerAddOptions', 'boxed': true }
>   
>   ##
>   # @NbdServerRemoveMode:
> @@ -167,6 +179,8 @@
>   # Describes a block export, i.e. how single node should be exported on an
>   # external interface.
>   #
> +# @device: The device name or node name of the node to be exported
> +#
>   # @writethrough: If true, caches are flushed after every write request to the
>   #                export before completion is signalled. (since: 5.2;
>   #                default: false)
> @@ -175,6 +189,7 @@
>   ##
>   { 'union': 'BlockExportOptions',
>     'base': { 'type': 'BlockExportType',
> +            'device': 'str',

for block export, I'd prefer that we mandate node name only, and naming 
it @node-name (rather than @device) feels nicer, for something that only 
new code will be using (that is, we should be encouraging the use of 
node names, especially now that libvirt has finally made that leap).
diff mbox series

Patch

diff --git a/qapi/block-export.json b/qapi/block-export.json
index 4ce163411f..d68f3bf87e 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -62,9 +62,8 @@ 
 ##
 # @BlockExportOptionsNbd:
 #
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
+# An NBD block export (options shared between nbd-server-add and the NBD branch
+# of block-export-add).
 #
 # @name: Export name. If unspecified, the @device parameter is used as the
 #        export name. (Since 2.12)
@@ -82,8 +81,21 @@ 
 # Since: 5.0
 ##
 { 'struct': 'BlockExportOptionsNbd',
-  'data': {'device': 'str', '*name': 'str', '*description': 'str',
-           '*writable': 'bool', '*bitmap': 'str' } }
+  'data': { '*name': 'str', '*description': 'str',
+            '*writable': 'bool', '*bitmap': 'str' } }
+
+##
+# @NbdServerAddOptions:
+#
+# An NBD block export.
+#
+# @device: The device name or node name of the node to be exported
+#
+# Since: 5.0
+##
+{ 'struct': 'NbdServerAddOptions',
+  'base': 'BlockExportOptionsNbd',
+  'data': { 'device': 'str' } }
 
 ##
 # @nbd-server-add:
@@ -96,7 +108,7 @@ 
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-add',
-  'data': 'BlockExportOptionsNbd', 'boxed': true }
+  'data': 'NbdServerAddOptions', 'boxed': true }
 
 ##
 # @NbdServerRemoveMode:
@@ -167,6 +179,8 @@ 
 # Describes a block export, i.e. how single node should be exported on an
 # external interface.
 #
+# @device: The device name or node name of the node to be exported
+#
 # @writethrough: If true, caches are flushed after every write request to the
 #                export before completion is signalled. (since: 5.2;
 #                default: false)
@@ -175,6 +189,7 @@ 
 ##
 { 'union': 'BlockExportOptions',
   'base': { 'type': 'BlockExportType',
+            'device': 'str',
             '*writethrough': 'bool' },
   'discriminator': 'type',
   'data': {
diff --git a/block/export/export.c b/block/export/export.c
index 1d5de564c7..ef86bf892b 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -68,15 +68,26 @@  void qmp_block_export_add(BlockExportOptions *export, Error **errp)
     blk_exp_add(export, errp);
 }
 
-void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
 {
     BlockExport *export;
     BlockDriverState *bs;
     BlockBackend *on_eject_blk;
 
-    BlockExportOptions export_opts = {
-        .type = BLOCK_EXPORT_TYPE_NBD,
-        .u.nbd = *arg,
+    BlockExportOptions *export_opts = g_new(BlockExportOptions, 1);
+    *export_opts = (BlockExportOptions) {
+        .type                   = BLOCK_EXPORT_TYPE_NBD,
+        .device                 = g_strdup(arg->device),
+        .u.nbd = {
+            .has_name           = arg->has_name,
+            .name               = g_strdup(arg->name),
+            .has_description    = arg->has_description,
+            .description        = g_strdup(arg->description),
+            .has_writable       = arg->has_writable,
+            .writable           = arg->writable,
+            .has_bitmap         = arg->has_bitmap,
+            .bitmap             = g_strdup(arg->bitmap),
+        },
     };
 
     /*
@@ -89,9 +100,9 @@  void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
         arg->writable = false;
     }
 
-    export = blk_exp_add(&export_opts, errp);
+    export = blk_exp_add(export_opts, errp);
     if (!export) {
-        return;
+        goto fail;
     }
 
     /*
@@ -102,4 +113,7 @@  void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
     if (on_eject_blk) {
         nbd_export_set_on_eject_blk(export, on_eject_blk);
     }
+
+fail:
+    qapi_free_BlockExportOptions(export_opts);
 }
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index a651954e16..6c823234a9 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -398,7 +398,7 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     Error *local_err = NULL;
     BlockInfoList *block_list, *info;
     SocketAddress *addr;
-    BlockExportOptionsNbd export;
+    NbdServerAddOptions export;
 
     if (writable && !all) {
         error_setg(&local_err, "-w only valid together with -a");
@@ -431,7 +431,7 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }
 
-        export = (BlockExportOptionsNbd) {
+        export = (NbdServerAddOptions) {
             .device         = info->value->device,
             .has_writable   = true,
             .writable       = writable,
@@ -458,7 +458,7 @@  void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;
 
-    BlockExportOptionsNbd export = {
+    NbdServerAddOptions export = {
         .device         = (char *) device,
         .has_name       = !!name,
         .name           = (char *) name,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index a8b7b785e7..5e97975c80 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -188,7 +188,7 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
     }
 
     if (!arg->has_name) {
-        arg->name = arg->device;
+        arg->name = exp_args->device;
     }
 
     if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
@@ -206,7 +206,7 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
         return NULL;
     }
 
-    bs = bdrv_lookup_bs(arg->device, arg->device, errp);
+    bs = bdrv_lookup_bs(exp_args->device, exp_args->device, errp);
     if (!bs) {
         return NULL;
     }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d967b8fcb9..f31868708c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1055,10 +1055,10 @@  int main(int argc, char **argv)
     export_opts = g_new(BlockExportOptions, 1);
     *export_opts = (BlockExportOptions) {
         .type               = BLOCK_EXPORT_TYPE_NBD,
+        .device             = g_strdup(bdrv_get_node_name(bs)),
         .has_writethrough   = true,
         .writethrough       = writethrough,
         .u.nbd = {
-            .device             = g_strdup(bdrv_get_node_name(bs)),
             .has_name           = true,
             .name               = g_strdup(export_name),
             .has_description    = !!export_description,