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

[Eric Blake]

On 01/18/2018 12:11 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add command for export removing. It is needed for cases when we

s/export removing/removing an export/

> 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.
> 

No HMP counterpart? I can give a quick shot at that, as a followup patch.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  qapi/block.json     | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/block/nbd.h |  1 +
>  blockdev-nbd.c      | 24 ++++++++++++++++++++++++
>  nbd/server.c        | 21 +++++++++++++++++++++
>  4 files changed, 91 insertions(+)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 353e3a45bd..ddf73932ce 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -228,6 +228,51 @@
>    'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @NbdServerRemoveMode:
> +#
> +# Mode of NBD export removing.

Mode for removing an NBD export.

> +#
> +# @safe: Remove export if there are no existing connections, fail otherwise.
> +#
> +# @hard: Drop all connections immediately and remove export.
> +#
> +# Postponed, not realized yet modes:

Maybe:

Potential additional modes to be added in the future:

> +#
> +# hide: Just hide export from new clients, leave existing connections as is.
> +#       Remove export after all clients are disconnected. nbd-server-remove
> +#       with mode=soft or mode=hard may be called after nbd-server-remove
> +#       with mode=hide.

mode=safe could also be called; I don't see that this last sentence adds
much.

> +#
> +# soft: Hide export from new clients, answer with ESHUTDOWN for all further
> +#       requests from existing clients. Remove export after all clients are
> +#       disconnected. nbd-server-requests with mode=hard may be called after
> +#       nbd-server-remove with mode=soft

Again, the last sentence doesn't add mouch (I guess you're arguing that
requesting a hide doesn't make much sense after a soft disconnect has
already started; but I don't see any harm in permitting it as a no-op).

> +#
> +# Since: 2.12
> +##
> +{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
> +
> +##
> +# @nbd-server-remove:
> +#
> +# Remove NBD export by name.
> +#
> +# @name: Export name.
> +#
> +# @mode: Mode of command operation. See @NbdServerRemoveMode description.
> +#        Default is 'safe'.
> +#
> +# Returns: error if
> +#            - the server is not running
> +#            - export is not found
> +#            - mode is 'safe' and there are existing connections
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove',
> +  'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }

Looks reasonable.


> +++ b/nbd/server.c
> @@ -1177,6 +1177,27 @@ void nbd_export_close(NBDExport *exp)
>      nbd_export_put(exp);
>  }
>  
> +void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error 
> **errp)
> +{
> +    NBDClient *client;
> +    int nb_clients = 0;
> +
> +    if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
> +        nbd_export_close(exp);
> +        return;
> +    }
> +
> +    assert(mode == NBD_SERVER_REMOVE_MODE_SAFE);
> +
> +    QTAILQ_FOREACH(client, &exp->clients, next) {
> +        nb_clients++;
> +    }
> +
> +    error_setg(errp, "NBD export '%s' has %d active connection%s. To force "
> +               "remove it (and hard disconnect clients) use mode='hard'",
> +               exp->name, nb_clients, nb_clients == 1 ? "" : "s");

Playing games with English pluralization doesn't work too well in error
messages, if we ever want to allow translations; does the number of
active clients actually matter?  Also, error_setg() recommends against
using '.' or more than one sentence.  Better might be:

error_setg(errp, "export '%s' still in use");
error_append_hint(errp, "Use mode='hard' to force client disconnect\n");

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

signature.asc
Description: OpenPGP digital signature