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

[Vladimir Sementsov-Ogievskiy]

09.01.2018 22:52, Eric Blake wrote:
> 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)?

looks interesting. what about the following naming?

@mode: possible values:
              hide - just hide server from new clients, maintain 
existing connections,
                          remove after all clients disconnected
              soft - like hide, but answer with ESHUTDOWN for all 
further requests from
                          existing connections
              hard - hard disconnect all clients and remove server
              (default: soft)

new corresponding states of nbd export:
hidden, shutting_down

and we allow transitions:

normal_execution -> hidden
normal_execution -> shutting_down
normal_execution -> exit
hidden -> shutting_down
hidden -> exit
shutting_down -> exit


> > +#
> > +# 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>


--
Best regards,
Vladimir