From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RyKzI-0006Z8-Oe for qemu-devel@nongnu.org; Fri, 17 Feb 2012 05:24:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RyKzE-0002n1-CD for qemu-devel@nongnu.org; Fri, 17 Feb 2012 05:24:24 -0500 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:60191) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RyKzE-0002mp-2l for qemu-devel@nongnu.org; Fri, 17 Feb 2012 05:24:20 -0500 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 17 Feb 2012 10:24:17 -0000 Received: from d06av12.portsmouth.uk.ibm.com (d06av12.portsmouth.uk.ibm.com [9.149.37.247]) by d06nrmr1407.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q1HAOFxx2539734 for ; Fri, 17 Feb 2012 10:24:15 GMT Received: from d06av12.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av12.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q1HAOEmQ002604 for ; Fri, 17 Feb 2012 03:24:15 -0700 Date: Fri, 17 Feb 2012 10:24:14 +0000 From: Stefan Hajnoczi Message-ID: <20120217102414.GA24722@stefanha-thinkpad.localdomain> References: <1329452408-13526-1-git-send-email-zwu.kernel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1329452408-13526-1-git-send-email-zwu.kernel@gmail.com> Subject: Re: [Qemu-devel] [PATCH] socket: add the support for -netdev socket, listen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zwu.kernel@gmail.com Cc: aliguori@us.ibm.com, Zhi Yong Wu , srini@cisco.com, qemu-devel@nongnu.org On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote: > From: Zhi Yong Wu > > As you have known, QEMU upstream currently doesn't support for -netdev socket,listen; This patch makes it work now. This commit description does not give any context. Please explain what the bug is so readers know what this patch fixes. I tried the following test: $ x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 1024 \ -drive if=virtio,file=vm1.img,cache=none \ -netdev socket,listen=127.0.0.1:1234,id=socket0 \ -device virtio-net-pci,netdev=socket0 $ x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 1024 \ -drive if=virtio,file=vm2.img,cache=none \ -netdev socket,connect=127.0.0.1:1234,id=socket0 \ -device virtio-net-pci,netdev=socket0 The first thing I noticed was that the output of "info network" in vm1 looks wrong. It should show the virtio-net-pci NIC peered with a socket fd connection. Instead it shows it peered with a dummy VLANClientState and I see two socket fds with no peers. Network connectivity between the two QEMUs does not work. I assigned static IPs in both VMs, ran tcpdump in vm1, and then tried to ping vm1 from vm2. > Signed-off-by: Zhi Yong Wu > --- > net/socket.c | 17 +++++++++++++++++ > 1 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index d4c2002..f82e69d 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -43,6 +43,7 @@ typedef struct NetSocketState { > } NetSocketState; > > typedef struct NetSocketListenState { > + VLANClientState *nc; > VLANState *vlan; > char *model; > char *name; > @@ -389,6 +390,11 @@ static void net_socket_accept(void *opaque) > break; > } > } > + > + if (s->nc) { > + qemu_del_vlan_client(s->nc); > + } > + > s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); This has a few issues: net_socket_fd_init() does not set the peer, only vlan. This means the connected socket and NIC are not peered up. qemu_del_vlan_client() brings the link down...we never bring it back up. We need to avoid leaking s->nc because it is not freed in qemu_del_vlan_client(). Once peering is fixed s->nc will not be freed during NIC deletion anymore! It leaves a dangling pointer to s->nc in the qdev netdev property and NICInfo nd_table[]. Not sure if this is a problem but it's messy. I suggest using the real NetSocketState instead - do not create a dummy VLANClientState. This means we create the socket fd NetSocketState right away and never need to update the peer. Simply bring the link up once the socket file descriptor is connected. Stefan