Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

On 12/07/2017 09:50 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 <address@hidden>
> ---
>  qapi/block.json     | 18 ++++++++++++++++++
>  include/block/nbd.h |  3 ++-
>  blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
>  nbd/server.c        | 25 ++++++++++++++++++++++---
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 503d4b287b..f83485c325 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -228,6 +228,24 @@
>    'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @nbd-server-remove:
> +#
> +# Remove NBD export by name.
> +#
> +# @name: Export name.
> +#
> +# @force: Whether active connections to the export should be closed. If this
> +#         parameter is false the export only becomes hidden from clients (new
> +#         connections are impossible), and would be automatically freed after
> +#         all clients are disconnected (default false).

Unstated, but if the parameter is true, existing clients are forcefully
disconnected, possibly losing pending transactions.

Do we want a third mode in the middle, where the server starts replying
to all existing clients with ESHUTDOWN errors for all new requests
rather than abruptly disconnecting (no new I/O, but no forced disconnect
and pending in-flight transactions can still complete gracefully)?

> +#
> +# Returns: error if the server is not running or export is not found.
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove', 'data': {'name': 'str', '*force': 'bool'} }
> +

If we're okay with just the bool parameter, then this patch looks good;
but if we want a third mode, then we want '*force' to be an enum type.
So tentative:

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature