[v2,2/6] qapi: add name parameter to nbd-server-add

Message ID 20171207155102.66622-3-vsementsov@virtuozzo.com
State New
Headers show
Series
  • nbd export qmp interface
Related show

Commit Message

Vladimir Sementsov-Ogievskiy Dec. 7, 2017, 3:50 p.m.
Allow user to specify name for new export, to not reuse internal
node name and to not show it to clients.

This also allows creating several exports per device.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json |  9 +++++++--
 blockdev-nbd.c  | 14 +++++++++-----
 hmp.c           |  5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

Comments

Dr. David Alan Gilbert Dec. 8, 2017, 5:33 p.m. | #1
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
> 
> This also allows creating several exports per device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json |  9 +++++++--
>  blockdev-nbd.c  | 14 +++++++++-----
>  hmp.c           |  5 +++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
>  #
>  # @device: The device name or node name of the node to be exported
>  #
> +# @name: Export name. If unspecified @device parameter used as export name.
> +#        (Since 2.12)
> +#
>  # @writable: Whether clients should be able to write to the device via the
>  #     NBD connection (default false).
>  #
> -# Returns: error if the device is already marked for export.
> +# Returns: error if the server is not running, or export with the same name
> +#          already exists.
>  #
>  # Since: 1.3.0
>  ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +{ 'command': 'nbd-server-add',
> +  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
>  # @nbd-server-stop:
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 28f551a7b0..46c885aa35 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
>      qapi_free_SocketAddress(addr_flat);
>  }
>  
> -void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
> -                        Error **errp)
> +void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> +                        bool has_writable, bool writable, Error **errp)
>  {
>      BlockDriverState *bs = NULL;
>      BlockBackend *on_eject_blk;
> @@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          return;
>      }
>  
> -    if (nbd_export_find(device)) {
> -        error_setg(errp, "NBD server already exporting device '%s'", device);
> +    if (!has_name) {
> +        name = device;
> +    }
> +
> +    if (nbd_export_find(name)) {
> +        error_setg(errp, "NBD server already has export named '%s'", name);
>          return;
>      }
>  
> @@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          return;
>      }
>  
> -    nbd_export_set_name(exp, device);
> +    nbd_export_set_name(exp, name);
>  
>      /* The list of named exports has a strong reference to this export now and
>       * our only way of accessing it is through nbd_export_find(), so we can drop
> diff --git a/hmp.c b/hmp.c
> index 35a7041824..0ea9c09b58 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>              continue;
>          }
>  
> -        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
> +        qmp_nbd_server_add(info->value->device, false, NULL,
> +                           true, writable, &local_err);
>  
>          if (local_err != NULL) {
>              qmp_nbd_server_stop(NULL);
> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>      bool writable = qdict_get_try_bool(qdict, "writable", false);
>      Error *local_err = NULL;
>  
> -    qmp_nbd_server_add(device, true, writable, &local_err);
> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);

I wont insist, but it would be nice if you wired up an optional
parameter on HMP as well.

Dave

>      if (local_err != NULL) {
>          hmp_handle_error(mon, &local_err);
> -- 
> 2.11.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Vladimir Sementsov-Ogievskiy Dec. 9, 2017, 9:28 a.m. | #2
08.12.2017 20:33, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> Allow user to specify name for new export, to not reuse internal
>> node name and to not show it to clients.
>>
>> This also allows creating several exports per device.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block.json |  9 +++++++--
>>   blockdev-nbd.c  | 14 +++++++++-----
>>   hmp.c           |  5 +++--
>>   3 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index f093fa3f27..503d4b287b 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -213,14 +213,19 @@
>>   #
>>   # @device: The device name or node name of the node to be exported
>>   #
>> +# @name: Export name. If unspecified @device parameter used as export name.
>> +#        (Since 2.12)
>> +#
>>   # @writable: Whether clients should be able to write to the device via the
>>   #     NBD connection (default false).
>>   #
>> -# Returns: error if the device is already marked for export.
>> +# Returns: error if the server is not running, or export with the same name
>> +#          already exists.
>>   #
>>   # Since: 1.3.0
>>   ##
>> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
>> +{ 'command': 'nbd-server-add',
>> +  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>>   
>>   ##
>>   # @nbd-server-stop:
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 28f551a7b0..46c885aa35 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
>>       qapi_free_SocketAddress(addr_flat);
>>   }
>>   
>> -void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>> -                        Error **errp)
>> +void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>> +                        bool has_writable, bool writable, Error **errp)
>>   {
>>       BlockDriverState *bs = NULL;
>>       BlockBackend *on_eject_blk;
>> @@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>           return;
>>       }
>>   
>> -    if (nbd_export_find(device)) {
>> -        error_setg(errp, "NBD server already exporting device '%s'", device);
>> +    if (!has_name) {
>> +        name = device;
>> +    }
>> +
>> +    if (nbd_export_find(name)) {
>> +        error_setg(errp, "NBD server already has export named '%s'", name);
>>           return;
>>       }
>>   
>> @@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>           return;
>>       }
>>   
>> -    nbd_export_set_name(exp, device);
>> +    nbd_export_set_name(exp, name);
>>   
>>       /* The list of named exports has a strong reference to this export now and
>>        * our only way of accessing it is through nbd_export_find(), so we can drop
>> diff --git a/hmp.c b/hmp.c
>> index 35a7041824..0ea9c09b58 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>>               continue;
>>           }
>>   
>> -        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
>> +        qmp_nbd_server_add(info->value->device, false, NULL,
>> +                           true, writable, &local_err);
>>   
>>           if (local_err != NULL) {
>>               qmp_nbd_server_stop(NULL);
>> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>>       bool writable = qdict_get_try_bool(qdict, "writable", false);
>>       Error *local_err = NULL;
>>   
>> -    qmp_nbd_server_add(device, true, writable, &local_err);
>> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> I wont insist, but it would be nice if you wired up an optional
> parameter on HMP as well.
>
> Dave

Hmm.. Honestly, I don't see real usage of it.

>
>>       if (local_err != NULL) {
>>           hmp_handle_error(mon, &local_err);
>> -- 
>> 2.11.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Jan. 9, 2018, 7:05 p.m. | #3
On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
> 
> This also allows creating several exports per device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json |  9 +++++++--
>  blockdev-nbd.c  | 14 +++++++++-----
>  hmp.c           |  5 +++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
>  #
>  # @device: The device name or node name of the node to be exported
>  #
> +# @name: Export name. If unspecified @device parameter used as export name.

s/unspecified/unspecified, the/
s/used as/is used as the/

Will tweak as part of applying to my NBD queue.
Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake Jan. 9, 2018, 7:06 p.m. | #4
On 12/08/2017 11:33 AM, Dr. David Alan Gilbert wrote:

>> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>>      bool writable = qdict_get_try_bool(qdict, "writable", false);
>>      Error *local_err = NULL;
>>  
>> -    qmp_nbd_server_add(device, true, writable, &local_err);
>> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> 
> I wont insist, but it would be nice if you wired up an optional
> parameter on HMP as well.

Can be done as a followup patch; I'm not sure how many people are
setting up NBD exports via HMP, and I'm also okay with just stating that
the full power requires use of QMP.  But I'll give such a followup patch
a quick try, to see whether it is easy after all.
Dr. David Alan Gilbert Jan. 10, 2018, 4:01 p.m. | #5
* Eric Blake (eblake@redhat.com) wrote:
> On 12/08/2017 11:33 AM, Dr. David Alan Gilbert wrote:
> 
> >> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
> >>      bool writable = qdict_get_try_bool(qdict, "writable", false);
> >>      Error *local_err = NULL;
> >>  
> >> -    qmp_nbd_server_add(device, true, writable, &local_err);
> >> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> > 
> > I wont insist, but it would be nice if you wired up an optional
> > parameter on HMP as well.
> 
> Can be done as a followup patch

Yes, agreed.

> I'm not sure how many people are
> setting up NBD exports via HMP, and I'm also okay with just stating that
> the full power requires use of QMP.  But I'll give such a followup patch
> a quick try, to see whether it is easy after all.

Thanks; I have set up NBD exports up via HMP a few times.

Dave

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patch

diff --git a/qapi/block.json b/qapi/block.json
index f093fa3f27..503d4b287b 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -213,14 +213,19 @@ 
 #
 # @device: The device name or node name of the node to be exported
 #
+# @name: Export name. If unspecified @device parameter used as export name.
+#        (Since 2.12)
+#
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false).
 #
-# Returns: error if the device is already marked for export.
+# Returns: error if the server is not running, or export with the same name
+#          already exists.
 #
 # Since: 1.3.0
 ##
-{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+{ 'command': 'nbd-server-add',
+  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
 
 ##
 # @nbd-server-stop:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 28f551a7b0..46c885aa35 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -158,8 +158,8 @@  void qmp_nbd_server_start(SocketAddressLegacy *addr,
     qapi_free_SocketAddress(addr_flat);
 }
 
-void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
-                        Error **errp)
+void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
+                        bool has_writable, bool writable, Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockBackend *on_eject_blk;
@@ -170,8 +170,12 @@  void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    if (nbd_export_find(device)) {
-        error_setg(errp, "NBD server already exporting device '%s'", device);
+    if (!has_name) {
+        name = device;
+    }
+
+    if (nbd_export_find(name)) {
+        error_setg(errp, "NBD server already has export named '%s'", name);
         return;
     }
 
@@ -195,7 +199,7 @@  void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    nbd_export_set_name(exp, device);
+    nbd_export_set_name(exp, name);
 
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
diff --git a/hmp.c b/hmp.c
index 35a7041824..0ea9c09b58 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2203,7 +2203,8 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }
 
-        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
+        qmp_nbd_server_add(info->value->device, false, NULL,
+                           true, writable, &local_err);
 
         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -2223,7 +2224,7 @@  void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;
 
-    qmp_nbd_server_add(device, true, writable, &local_err);
+    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
 
     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);