add qmp nbd-server-remove
cover add qmp nbd-server-remove
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(+)
Signature · 1 line
2.11.1
PATCH 1/2 nbd/server: add additional assert to nbd_export_put
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.
--- nbd/server.c | 1 + 1 file changed, 1 insertion(+)
nbd/server.c +1 -0
@@ -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 );
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 1/2] nbd/server: add additional assert to nbd_export_put
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
This place is not obvious,
nbd_export_closemay theoretically reduce refcount to 0. It may happen if someone callsnbd_export_puton named export not throughnbd_export_set_namewhen refcount is 1.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- nbd/server.c | 1 + 1 file changed, 1 insertion(+)
PATCH 2/2 qmp: add nbd-server-remove
In reply to Vladimir Sementsov-OgievskiyAdd 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.
--- qapi/block.json | 20 ++++++++++++++++++++ blockdev-nbd.c | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
qapi/block.json +20 -0
@@ -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
blockdev-nbd.c +27 -0
@@ -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 ();
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove
Quoted 11 lines · 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 <[email protected]>--- 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.
Quoted 7 lines
##
qapi/block.json +6
-0+# @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.
qapi/block.json line 241
Quoted 10 lines
qapi/block.json +10
-0+# +# @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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove
In reply to Vladimir Sementsov-OgievskiyQuoted 11 lines · 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 <[email protected]>--- qapi/block.json | 20 ++++++++++++++++++++ blockdev-nbd.c | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
Code looks good, just some documentation remarks:
qapi/block.json line 228
Quoted 11 lines
qapi/block.json +3
-0@@ -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/
qapi/block.json line 231
Quoted 3 lines
qapi/block.json +3
-0+# +# @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).
qapi/block.json line 235
Quoted 4 lines
qapi/block.json +4
-0+# +# @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
qapi/block.json line 239
Quoted 4 lines
qapi/block.json +4
-0+# 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
Quoted 48 lines
qapi/block.json +6
-0+# +# 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 previouslyblockdev-nbd.c +27
-0@@ -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 ();
Re: [Qemu-devel] [PATCH 2/2] qmp: add nbd-server-remove
In reply to Vladimir Sementsov-OgievskiyAm 09.11.2017 um 16:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 67 lines
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 <[email protected]>--- qapi/block.json | 20 ++++++++++++++++++++ blockdev-nbd.c | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)qapi/block.json +20
-0@@ -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 previouslyblockdev-nbd.c +16
-0@@ -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 ; + } +
blockdev-nbd.c void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, · line 231
Quoted 10 lines
blockdev-nbd.c +10
-0+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
Quoted 74 lines · Kevin Wolf
Am 09.11.2017 um 16:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 73 lines
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 <[email protected]>--- qapi/block.json | 20 ++++++++++++++++++++ blockdev-nbd.c | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)qapi/block.json +20
-0@@ -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 previouslyblockdev-nbd.c +22
-0@@ -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);
Quoted 8 lines
blockdev-nbd.c void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, · line 231
Quoted 4 lines
blockdev-nbd.c +4
-0+ }else { +nbd_export_set_name (exp,NULL ); + } +}Using
nbd_export_find()in order to identify the export, andnbd_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.
Quoted 7 lines
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.
Quoted 9 lines
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).
Quoted 4 lines
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.
Quoted 2 lines
Kevin
Signature · 2 lines
Best regards, Vladimir
Quoted 11 lines · 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 tonbd_export_put
qmp: add nbd-server-removeqapi/block.json | 20 ++++++++++++++++++++ blockdev-nbd.c | 27 +++++++++++++++++++++++++++ nbd/server.c | 1 + 3 files changed, 48 insertions(+)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org