Re: [Qemu-devel] [PATCH] socket: add the support for -netdev socket, listen

[Zhi Yong Wu]

On Sat, Feb 18, 2012 at 6:06 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Sat, Feb 18, 2012 at 5:35 AM, Zhi Yong Wu <address@hidden> wrote:
>> On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi
>> <address@hidden> wrote:
>>> On Fri, Feb 17, 2012 at 12:20:08PM +0800, address@hidden wrote:
>>>> From: Zhi Yong Wu <address@hidden>
>>>>
>>>> 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