From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60488) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RyhBc-00074t-SB for qemu-devel@nongnu.org; Sat, 18 Feb 2012 05:06:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RyhBb-00077y-9d for qemu-devel@nongnu.org; Sat, 18 Feb 2012 05:06:36 -0500 Received: from mail-lpp01m010-f45.google.com ([209.85.215.45]:35374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RyhBb-00077m-2h for qemu-devel@nongnu.org; Sat, 18 Feb 2012 05:06:35 -0500 Received: by lahi5 with SMTP id i5so5114227lah.4 for ; Sat, 18 Feb 2012 02:06:34 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1329452408-13526-1-git-send-email-zwu.kernel@gmail.com> <20120217102414.GA24722@stefanha-thinkpad.localdomain> Date: Sat, 18 Feb 2012 10:06:34 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: Zhi Yong Wu Cc: aliguori@us.ibm.com, Zhi Yong Wu , srini@cisco.com, Stefan Hajnoczi , qemu-devel@nongnu.org On Sat, Feb 18, 2012 at 5:35 AM, Zhi Yong Wu wrote: > On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi > wrote: >> 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. =A0Please explain wha= t >> 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 \ >> =A0-drive if=3Dvirtio,file=3Dvm1.img,cache=3Dnone \ >> =A0-netdev socket,listen=3D127.0.0.1:1234,id=3Dsocket0 \ >> =A0-device virtio-net-pci,netdev=3Dsocket0 >> >> $ x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 1024 \ >> =A0-drive if=3Dvirtio,file=3Dvm2.img,cache=3Dnone \ >> =A0-netdev socket,connect=3D127.0.0.1:1234,id=3Dsocket0 \ >> =A0-device virtio-net-pci,netdev=3Dsocket0 >> >> The first thing I noticed was that the output of "info network" in vm1 >> looks wrong. =A0It should show the virtio-net-pci NIC peered with a sock= et >> fd connection. =A0Instead it shows it peered with a dummy VLANClientStat= e > Sorry, i will correct it. >> and I see two socket fds with no peers. > It seems that other network backends don't set their peers, such as > slirp, tap, etc. Right. Normally the backend doesn't because setting the peer is only done once and it is bi-directional. Setting the peer on A will do: A->peer =3D B; B->peer =3D A; However, the reason that backends don't set it is because the NIC will find the -netdev and peer with it. This doesn't apply to your patch - you decided to create a dummy VLANClientState and then switch to a different VLANClientState. So what happens is that the NIC probably peers with the dummy VLANClientState. Then later on the socket connection is accepted so you now would need to re-peer. This is the reason why I think the dummy VLANClientState approach is difficult and it's cleaner to create the real VLANClientState right away. >> qemu_del_vlan_client() brings the link down...we never bring it back up. > Since s->nc->peer is NULL, this function will not bring the link down. > The default state of the link is up. The peer is non-NULL when -netdev/-device is used because the NIC peers with the netdev. >> We need to avoid leaking s->nc because it is not freed in > qemu_del_vlan_client->qemu_free_vlan_client->g_free(s->nc), i think > that it is freed, right? No. When -netdev/-device is used we will have a peer and it will be NET_CLIENT_TYPE_NIC. We go down a code path which sets nic->peer_deleted =3D true, brings the link down, and cleans up the dummy VLANClientState without freeing it. >> I suggest using the real NetSocketState instead - do not create a dummy >> VLANClientState. =A0This means we create the socket fd NetSocketState > Sorry, i get confused about "fd". Is this fd returned by "socket()" or > "accept()"? I meant net/socket.c when I said "socket fd" but the sentence makes sense if we drop that completely: "We create the NetSocketState right away and never need to update the peer.= " > If we directly create one real NetSocketState, the code change will be > a bit large, right? net_socket_info only implements .receive and .cleanup, and net/socket.c is not a large file. It should be doable in a clean and reasonable way. Stefan