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

[Max Reitz]

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 <address@hidden>
> ---
>  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();
>

signature.asc
Description: OpenPGP digital signature