Message ID | 20171109154049.42386-3-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | None | expand |
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.
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(); >
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
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
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();
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(+)