On Sat, Feb 18, 2012 at 5:35 AM, Zhi Yong Wu <zwu.ker...@gmail.com> wrote: > On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi > <stefa...@linux.vnet.ibm.com> wrote: >> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.ker...@gmail.com wrote: >>> From: Zhi Yong Wu <wu...@linux.vnet.ibm.com> >>> >>> 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 > 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 = B; B->peer = 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 = 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. This 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