From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46921)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1VgxN4-0007b5-Ae
	for qemu-devel@nongnu.org; Thu, 14 Nov 2013 08:54:16 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1VgxMy-0005Tu-As
	for qemu-devel@nongnu.org; Thu, 14 Nov 2013 08:54:10 -0500
Received: from mx1.redhat.com ([209.132.183.28]:50209)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1VgxMy-0005To-1S
	for qemu-devel@nongnu.org; Thu, 14 Nov 2013 08:54:04 -0500
Date: Thu, 14 Nov 2013 15:56:58 +0200
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20131114135658.GC5658@redhat.com>
References: <1384273995-16486-1-git-send-email-cminyard@mvista.com>
	<1384273995-16486-3-git-send-email-cminyard@mvista.com>
	<1384332943.3560.18.camel@nilsson.home.kraxel.org>
	<5283E65B.9020908@mvista.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5283E65B.9020908@mvista.com>
Subject: Re: [Qemu-devel] [PATCH 02/16] qemu-char: Allow a chardev to
 reconnect if disconnected
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Corey Minyard <cminyard@mvista.com>
Cc: Bret Ketchum <bcketchum@gmail.com>, Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>, Gerd Hoffmann <kraxel@redhat.com>, Corey Minyard <minyard@acm.org>, qemu-devel@nongnu.org

On Wed, Nov 13, 2013 at 02:51:39PM -0600, Corey Minyard wrote:
> On 11/13/2013 02:55 AM, Gerd Hoffmann wrote:
> >   Hi,
> >
> >> Allow a socket that connects to reconnect on a periodic basis if it
> >> fails to connect at startup or if the connection drops while in use.
> >> +    chr->backend = i;
> >> +    chr->recon_time = qemu_opt_get_number(opts, "reconnect", 0);
> >> +    if (chr->recon_time) {
> >> +        if (strcmp(qemu_opt_get(opts, "backend"), "socket") != 0) {
> >> +            g_free(chr);
> >> +            fprintf(stderr, "chardev: reconnect only supported on sockets\n");
> >> +            return NULL;
> >> +        }
> > I think it would work *much* better to just add a "chr_reconnect"
> > function pointer to CharDriverState.  You don't need patch #1 then.
> > Also it avoids backend-specific bits in generic code.  Generic code just
> > checks if chr->chr_reconnect exists to figure whenever reconnect is
> > supported or not.  And the socket backend can set chr_reconnect for
> > client sockets only.
> 
> I was trying for something more generic than one that just applies to
> socket devices.  It may not be good for server sockets, but it might be
> useful for ptys and hotplug devices.  Looking at the code, ptys already
> has its own code that does something similar; that could be pulled out
> and replaced with the generic code.
> 
> Also, IMHO allocating the chardev in each back end is not optimal.  It
> breaks layering.  Plus patch #1 has a net reduction in lines of code
> because it pulls out all the allocations and does it in one place.
> 
> Thanks,
> 
> -corey

I guess this was just one suggestion.

What Gerd is asking for here is that we avoid
code like

	strcmp(qemu_opt_get(opts, "backend"), "socket")

in generic code, instead making a backend report
reconnect support.
This sounds reasonable, does it not?