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

[Corey Minyard <minyard@acm.org>]

On 09/22/2014 03:24 PM, Eric Blake wrote:
> 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,

Point taken...

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

Ok, I'll fix this.

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

I was going on the behavior of "telnet" and "wait", which are silently
ignored for client sockets.  reconnect is silently ignored for server
sockets.

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

Doesn't seem terrible, but I'm not sure.

-corey