From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:47576)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XV6kD-0001os-9w
	for qemu-devel@nongnu.org; Fri, 19 Sep 2014 18:33:41 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XV6k7-0007bu-3F
	for qemu-devel@nongnu.org; Fri, 19 Sep 2014 18:33:37 -0400
Received: from mail-wg0-x229.google.com ([2a00:1450:400c:c00::229]:44556)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XV6k6-0007Xu-TG
	for qemu-devel@nongnu.org; Fri, 19 Sep 2014 18:33:31 -0400
Received: by mail-wg0-f41.google.com with SMTP id k14so18725wgh.0
	for <qemu-devel@nongnu.org>; Fri, 19 Sep 2014 15:33:23 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <541CAF2E.2030502@redhat.com>
Date: Sat, 20 Sep 2014 00:33:18 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1411163937-19891-1-git-send-email-minyard@acm.org>
In-Reply-To: <1411163937-19891-1-git-send-email-minyard@acm.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH] chardev: Add reconnecting to client sockets
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: minyard@acm.org, qemu-devel@nongnu.org
Cc: mjg59@srcf.ucam.org, mst@redhat.com, hwd@huawei.com, bcketchum@gmail.com, Corey Minyard <cminyard@mvista.com>, afaerber@suse.de

Il 19/09/2014 23:58, minyard@acm.org ha scritto:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Adds a "recon" 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.
> 
> This rearranges things a bit, all socket configuration is moved to
> qmp_chardev_open_socket() and that only gets called once at startup.
> qemu_chr_open_socket_fd() is called to open or re-open the connection.

Please do this in a separate patch.

Also:

- qemu-char.c for various reasons is not using QEMU timers.  Please use
glib timers instead.

> +# @recon: #optional If not a server socket, if the socket disconnect
> +#          then reconnect after the given number of seconds.  Setting
> +#          to zero disables this function. (default: 0)

Please specify "Since: 2.2" inside the comment, and give a more
descriptive name such as reconnect-interval to the option.

> +	s->addr = addr; /* Steal the address from the socket */
> +	sock->addr = NULL;

Please do a deep copy instead.  (The sock struct should really be
const).  A function like this will do it for you:

    *p_dest = NULL;

    qov = qmp_output_visitor_new();
    ov = qmp_output_get_visitor(qov);
    visit_type_SocketAddress(ov, &src, NULL, errp);
    obj = qmp_output_get_qobject(data->qov);
    qmp_output_visitor_cleanup(qov);
    if (!obj) {
        return;
    }

    qiv = qmp_input_visitor_new(obj);
    iv = qmp_input_get_visitor(qiv);
    visit_type_SocketAddress(iv, p_dest, NULL, &error_abort);
    qmp_input_visitor_cleanup(qiv);
    qobject_decref(obj);

    ...

    qapi_copy_SocketAddress(&s->addr, sock->addr, &error_abort);

(Something like this should really be automatically generated for all
types...).

I only skimmed the code, but nothing else jumped up.

Paolo