[Qemu-devel] [PATCH 0/2] add qmp nbd-server-remove

[Qemu-devel] [PATCH 0/2] add qmp nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

Add command to remove nbd export, pair to nbd-server-add.
The whole thing and description are in patch 02.

Vladimir Sementsov-Ogievskiy (2):
  nbd/server: add additional assert to nbd_export_put
  qmp: add nbd-server-remove

 qapi/block.json | 20 ++++++++++++++++++++
 blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
 nbd/server.c    |  1 +
 3 files changed, 48 insertions(+)

-- 
2.11.1

[Qemu-devel] [PATCH 1/2] nbd/server: add additional assert to nbd_export_put

[Vladimir Sementsov-Ogievskiy]

This place is not obvious, nbd_export_close may theoretically reduce
refcount to 0. It may happen if someone calls nbd_export_put on named
export not through nbd_export_set_name when refcount is 1.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nbd/server.c b/nbd/server.c
index 70b40ed27e..2f2e05943f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1179,6 +1179,7 @@ void nbd_export_put(NBDExport *exp)
         nbd_export_close(exp);
     }
 
+    assert(exp->refcount > 0);
     if (--exp->refcount == 0) {
         assert(exp->name == NULL);
         assert(exp->description == NULL);
-- 
2.11.1

[Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

Add command for export removing. It is needed for cases when we
don't want to keep export after the operation on it was completed.
The other example is temporary node, created with blockdev-add.
If we want to delete it we should firstly remove corresponding
NBD export.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json | 20 ++++++++++++++++++++
 blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/qapi/block.json b/qapi/block.json
index f093fa3f27..1827940717 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -223,6 +223,26 @@
 { 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
 
 ##
+# @nbd-server-remove:
+#
+# Stop exporting block node through QEMU's embedded NBD server.
+#
+# @device: The device name or node name of the exported node. Should be equal
+#          to @device parameter for corresponding nbd-server-add command call.
+#
+# @force: Whether active connections to the export should be closed. If this
+#         parameter is false the export is only removed from named exports list,
+#         so new connetions are impossible and it would be freed after all
+#         clients are disconnected (default false).
+#
+# Returns: error if the server is not running or the device is not marked for
+#          export.
+#
+# Since: 2.12
+##
+{ 'command': 'nbd-server-remove', 'data': {'device': 'str', '*force': 'bool'} }
+
+##
 # @nbd-server-stop:
 #
 # Stop QEMU's embedded NBD server, and unregister all devices previously
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 28f551a7b0..5f66951c33 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -203,6 +203,33 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     nbd_export_put(exp);
 }
 
+void qmp_nbd_server_remove(const char *device, bool has_force, bool force,
+                           Error **errp)
+{
+    NBDExport *exp;
+
+    if (!nbd_server) {
+        error_setg(errp, "NBD server not running");
+        return;
+    }
+
+    exp = nbd_export_find(device);
+    if (exp == NULL) {
+        error_setg(errp, "'%s' is not exported", device);
+        return;
+    }
+
+    if (!has_force) {
+        force = false;
+    }
+
+    if (force) {
+        nbd_export_close(exp);
+    } else {
+        nbd_export_set_name(exp, NULL);
+    }
+}
+
 void qmp_nbd_server_stop(Error **errp)
 {
     nbd_export_close_all();
-- 
2.11.1

Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On 11/09/2017 09:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add command for export removing. It is needed for cases when we
> don't want to keep export after the operation on it was completed.
> The other example is temporary node, created with blockdev-add.
> If we want to delete it we should firstly remove corresponding
> NBD export.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json | 20 ++++++++++++++++++++
>  blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)

It would be nice (okay as a followup patch) to add iotest coverage of
the new command.

>  ##
> +# @nbd-server-remove:
> +#
> +# Stop exporting block node through QEMU's embedded NBD server.
> +#
> +# @device: The device name or node name of the exported node. Should be equal
> +#          to @device parameter for corresponding nbd-server-add command call.
> +#
> +# @force: Whether active connections to the export should be closed. If this
> +#         parameter is false the export is only removed from named exports list,
> +#         so new connetions are impossible and it would be freed after all
> +#         clients are disconnected (default false).
> +#
> +# Returns: error if the server is not running or the device is not marked for
> +#          export.
> +#
> +# Since: 2.12

You are correct that this is too late for 2.11, but I like the concept.
Once we have some testsuite coverage, I'll queue it for 2.12.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 0/2] add qmp nbd-server-remove

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On 11/09/2017 09:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add command to remove nbd export, pair to nbd-server-add.
> The whole thing and description are in patch 02.
> 
> Vladimir Sementsov-Ogievskiy (2):
>   nbd/server: add additional assert to nbd_export_put
>   qmp: add nbd-server-remove
> 
>  qapi/block.json | 20 ++++++++++++++++++++
>  blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
>  nbd/server.c    |  1 +
>  3 files changed, 48 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] nbd/server: add additional assert to nbd_export_put

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On 2017-11-09 16:40, Vladimir Sementsov-Ogievskiy wrote:
> This place is not obvious, nbd_export_close may theoretically reduce
> refcount to 0. It may happen if someone calls nbd_export_put on named
> export not through nbd_export_set_name when refcount is 1.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 1 +
>  1 file changed, 1 insertion(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

On 2017-11-09 16:40, Vladimir Sementsov-Ogievskiy wrote:
> Add command for export removing. It is needed for cases when we
> don't want to keep export after the operation on it was completed.
> The other example is temporary node, created with blockdev-add.
> If we want to delete it we should firstly remove corresponding
> NBD export.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json | 20 ++++++++++++++++++++
>  blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)

Code looks good, just some documentation remarks:

> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..1827940717 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -223,6 +223,26 @@
>  { 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @nbd-server-remove:
> +#
> +# Stop exporting block node through QEMU's embedded NBD server.

s/exporting block node/exporting a block node/

> +#
> +# @device: The device name or node name of the exported node. Should be equal
> +#          to @device parameter for corresponding nbd-server-add command call.

s/Should/Must/

This is indeed not the name of the node, but it is just the export name.
 Maybe the comment should more clearly say so (e.g. omit the first
sentence and change the second to "The export name, i.e. what has been
given to nbd-server-add as @device" or something).

> +#
> +# @force: Whether active connections to the export should be closed. If this
> +#         parameter is false the export is only removed from named exports list,
> +#         so new connetions are impossible and it would be freed after all

*connections

> +#         clients are disconnected (default false).
> +#
> +# Returns: error if the server is not running or the device is not marked for
> +#          export.

Maybe "[...] or there is no such NBD export."

(Because you can have a node attached to a device, specify the node name
for nbd-server-add and then you can't remove the export with the device
name even though technically that block device is exported)

Max

> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove', 'data': {'device': 'str', '*force': 'bool'} }
> +
> +##
>  # @nbd-server-stop:
>  #
>  # Stop QEMU's embedded NBD server, and unregister all devices previously
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 28f551a7b0..5f66951c33 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -203,6 +203,33 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>      nbd_export_put(exp);
>  }
>  
> +void qmp_nbd_server_remove(const char *device, bool has_force, bool force,
> +                           Error **errp)
> +{
> +    NBDExport *exp;
> +
> +    if (!nbd_server) {
> +        error_setg(errp, "NBD server not running");
> +        return;
> +    }
> +
> +    exp = nbd_export_find(device);
> +    if (exp == NULL) {
> +        error_setg(errp, "'%s' is not exported", device);
> +        return;
> +    }
> +
> +    if (!has_force) {
> +        force = false;
> +    }
> +
> +    if (force) {
> +        nbd_export_close(exp);
> +    } else {
> +        nbd_export_set_name(exp, NULL);
> +    }
> +}
> +
>  void qmp_nbd_server_stop(Error **errp)
>  {
>      nbd_export_close_all();
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove

[Kevin Wolf]

Am 09.11.2017 um 16:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add command for export removing. It is needed for cases when we
> don't want to keep export after the operation on it was completed.
> The other example is temporary node, created with blockdev-add.
> If we want to delete it we should firstly remove corresponding
> NBD export.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json | 20 ++++++++++++++++++++
>  blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..1827940717 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -223,6 +223,26 @@
>  { 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @nbd-server-remove:
> +#
> +# Stop exporting block node through QEMU's embedded NBD server.
> +#
> +# @device: The device name or node name of the exported node. Should be equal
> +#          to @device parameter for corresponding nbd-server-add command call.
> +#
> +# @force: Whether active connections to the export should be closed. If this
> +#         parameter is false the export is only removed from named exports list,
> +#         so new connetions are impossible and it would be freed after all
> +#         clients are disconnected (default false).
> +#
> +# Returns: error if the server is not running or the device is not marked for
> +#          export.
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove', 'data': {'device': 'str', '*force': 'bool'} }
> +
> +##
>  # @nbd-server-stop:
>  #
>  # Stop QEMU's embedded NBD server, and unregister all devices previously
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 28f551a7b0..5f66951c33 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -203,6 +203,33 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>      nbd_export_put(exp);
>  }
>  
> +void qmp_nbd_server_remove(const char *device, bool has_force, bool force,
> +                           Error **errp)
> +{
> +    NBDExport *exp;
> +
> +    if (!nbd_server) {
> +        error_setg(errp, "NBD server not running");
> +        return;
> +    }
> +
> +    exp = nbd_export_find(device);
> +    if (exp == NULL) {
> +        error_setg(errp, "'%s' is not exported", device);
> +        return;
> +    }
> +
> +    if (!has_force) {
> +        force = false;
> +    }
> +
> +    if (force) {
> +        nbd_export_close(exp);
> +    } else {
> +        nbd_export_set_name(exp, NULL);
> +    }
> +}

Using nbd_export_find() in order to identify the export, and
nbd_export_set_name(NULL) for force=false means that you can't follow up
with another nbd-server-remove,force=true later if the client just
doesn't want to go away voluntarily.

To be honest, I'm not very happy with the whole NBD interface, even as
it exists today. We're mixing up several things that should really be
kept separate: The export name (this should be configurable and the real
ID of the export that you use with things like nbd-server-remove), the
device/node name (just a pointer to the root node, not to be exposed or
even just stored anywhere) and whether the export is visible for clients.

nbd-server-remove with force=false should make the export invisible for
clients, but still keep the ID assigned so that another
nbd-server-remove with force=true can still be issued. We should also
really have a query-nbd-server that shows all exports with their IDs,
their root node (which may have changed meanwhile - snapshots, block
jobs, filter drivers) and information about the currently connected
client(s).

Maybe it would be worth to clean up the interfaces this way before we
add the first command that actually has to identify an export. The
existing commands should be easy to extend in a backwards compatible
way.

Kevin

Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

04.12.2017 15:32, Kevin Wolf wrote:
> Am 09.11.2017 um 16:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Add command for export removing. It is needed for cases when we
>> don't want to keep export after the operation on it was completed.
>> The other example is temporary node, created with blockdev-add.
>> If we want to delete it we should firstly remove corresponding
>> NBD export.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block.json | 20 ++++++++++++++++++++
>>   blockdev-nbd.c  | 27 +++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index f093fa3f27..1827940717 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -223,6 +223,26 @@
>>   { 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
>>   
>>   ##
>> +# @nbd-server-remove:
>> +#
>> +# Stop exporting block node through QEMU's embedded NBD server.
>> +#
>> +# @device: The device name or node name of the exported node. Should be equal
>> +#          to @device parameter for corresponding nbd-server-add command call.
>> +#
>> +# @force: Whether active connections to the export should be closed. If this
>> +#         parameter is false the export is only removed from named exports list,
>> +#         so new connetions are impossible and it would be freed after all
>> +#         clients are disconnected (default false).
>> +#
>> +# Returns: error if the server is not running or the device is not marked for
>> +#          export.
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'command': 'nbd-server-remove', 'data': {'device': 'str', '*force': 'bool'} }
>> +
>> +##
>>   # @nbd-server-stop:
>>   #
>>   # Stop QEMU's embedded NBD server, and unregister all devices previously
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 28f551a7b0..5f66951c33 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -203,6 +203,33 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>       nbd_export_put(exp);
>>   }
>>   
>> +void qmp_nbd_server_remove(const char *device, bool has_force, bool force,
>> +                           Error **errp)
>> +{
>> +    NBDExport *exp;
>> +
>> +    if (!nbd_server) {
>> +        error_setg(errp, "NBD server not running");
>> +        return;
>> +    }
>> +
>> +    exp = nbd_export_find(device);
>> +    if (exp == NULL) {
>> +        error_setg(errp, "'%s' is not exported", device);
>> +        return;
>> +    }
>> +
>> +    if (!has_force) {
>> +        force = false;
>> +    }
>> +
>> +    if (force) {
>> +        nbd_export_close(exp);
>> +    } else {
>> +        nbd_export_set_name(exp, NULL);
>> +    }
>> +}
> Using nbd_export_find() in order to identify the export, and
> nbd_export_set_name(NULL) for force=false means that you can't follow up
> with another nbd-server-remove,force=true later if the client just
> doesn't want to go away voluntarily.

reasonable, I'll think about it.

>
> To be honest, I'm not very happy with the whole NBD interface, even as
> it exists today. We're mixing up several things that should really be
> kept separate: The export name (this should be configurable and the real
> ID of the export that you use with things like nbd-server-remove), the
> device/node name (just a pointer to the root node, not to be exposed or
> even just stored anywhere) and whether the export is visible for clients.

yes. we have thought about this too, v3 will handle this problem.

>
> nbd-server-remove with force=false should make the export invisible for
> clients, but still keep the ID assigned so that another
> nbd-server-remove with force=true can still be issued. We should also
> really have a query-nbd-server that shows all exports with their IDs,
> their root node (which may have changed meanwhile - snapshots, block
> jobs, filter drivers) and information about the currently connected
> client(s).
>
> Maybe it would be worth to clean up the interfaces this way before we
> add the first command that actually has to identify an export. The
> existing commands should be easy to extend in a backwards compatible
> way.

yes, I've already started to do it.

>
> Kevin


-- 
Best regards,
Vladimir