Re: [Qemu-devel] [PATCH 5/6] qemu-char: Add reconnecting to client sockets

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

On 09/21/2014 05:04 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Adds a "reconnect" option to socket backends that gives a reconnect
> timeout.  This only applies to client sockets.  If the other end
> of a socket closes the connection, qemu will attempt to reconnect
> after the given number of seconds.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  qapi-schema.json | 14 +++++----
>  qemu-char.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  qemu-options.hx  | 20 ++++++++-----
>  3 files changed, 106 insertions(+), 17 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 689b548..79f7a07 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2648,14 +2648,18 @@
>  # @nodelay: #optional set TCP_NODELAY socket option (default: false)
>  # @telnet: #optional enable telnet protocol on server
>  #          sockets (default: false)
> +# @reconnect: #optional If not a server socket, if the socket disconnect

Awkward.  How about:

For a client socket, if a disconnect is detected,

> +#          then reconnect after the given number of seconds.  Setting
> +#          to zero disables this function. (default: 0). Since: 2.2.

I think this is usually written "(Since 2.2)" rather than "Since: 2.2"
when it occurs in the middle of a single option.

>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevSocket', 'data': { 'addr'     : 'SocketAddress',
> -                                     '*server'  : 'bool',
> -                                     '*wait'    : 'bool',
> -                                     '*nodelay' : 'bool',
> -                                     '*telnet'  : 'bool' } }
> +{ 'type': 'ChardevSocket', 'data': { 'addr'       : 'SocketAddress',
> +                                     '*server'    : 'bool',
> +                                     '*wait'      : 'bool',
> +                                     '*nodelay'   : 'bool',
> +                                     '*telnet'    : 'bool',
> +                                     '*reconnect' : 'int' } }

Hmm, thinking aloud here. What happens if 'reconnect' is provided with a
'server':true socket?  The documentation only specifies 'server':false
behavior.  Should it be an error (incompatible options), or just be
silently ignored?

Going further, would it be possible to treat 'ChardevSocket' as a flat
union, where 'server' is the enum key that determines what other fields
are valid?  Granted, for this to work, we'd need to teach the qapi
generator to allow a discriminator of type bool (since we can enumerate
all of its values). looking something like:

{ 'type': 'ChardevSocketBase',
  'data': { 'addr': 'SocketAddress', '*nodelay': 'bool' } }
{ 'type': 'ChardevSocketServer',
  'data': { '*wait': 'bool', '*telnet': 'bool' } }
{ 'type': 'ChardevSocketClient',
  'data': { '*reconnect': 'int' } }
{ 'union': 'ChardevSocket', 'base': 'ChardevSocketBase',
  'discriminator': 'bool',
  'data': { true : 'ChardevSocketServer',
            false: 'ChardevSocketClient' } }

but I don't know if it is worth the complexity for the added type safety.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]