From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35680) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWALS-0003GO-TD for qemu-devel@nongnu.org; Mon, 22 Sep 2014 16:36:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWALO-0008Og-8u for qemu-devel@nongnu.org; Mon, 22 Sep 2014 16:36:26 -0400 Received: from mail-oi0-x22d.google.com ([2607:f8b0:4003:c06::22d]:62421) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWALO-0008OA-1r for qemu-devel@nongnu.org; Mon, 22 Sep 2014 16:36:22 -0400 Received: by mail-oi0-f45.google.com with SMTP id g201so2883965oib.4 for ; Mon, 22 Sep 2014 13:36:16 -0700 (PDT) Sender: Corey Minyard Message-ID: <5420883E.6010303@acm.org> Date: Mon, 22 Sep 2014 15:36:14 -0500 From: Corey Minyard MIME-Version: 1.0 References: <1411340664-26912-1-git-send-email-minyard@acm.org> <1411340664-26912-6-git-send-email-minyard@acm.org> <5420857C.8050504@redhat.com> In-Reply-To: <5420857C.8050504@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] qemu-char: Add reconnecting to client sockets Reply-To: minyard@acm.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: mjg59@srcf.ucam.org, mst@redhat.com, hwd@huawei.com, bcketchum@gmail.com, Corey Minyard , afaerber@suse.de On 09/22/2014 03:24 PM, Eric Blake wrote: > On 09/21/2014 05:04 PM, minyard@acm.org wrote: >> From: Corey Minyard >> >> 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 >> --- >> 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