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

[Stefan Hajnoczi]

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.

Stefan