On Sat, Feb 18, 2012 at 6:06 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: > 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. nic, thanks.
> > Stefan -- Regards, Zhi Yong Wu
