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

[Zhi Yong Wu]

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
> and I see two socket fds with no peers.
By the way, Can you see socket file descriptioner? Where and How did
you see them?
>
> Network connectivity between the two QEMUs does not work.  I assigned
> static IPs in both VMs, ran tcpdump in vm1, and then tried to ping vm1
> from vm2.
>
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> ---
>>  net/socket.c |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index d4c2002..f82e69d 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -43,6 +43,7 @@ typedef struct NetSocketState {
>>  } NetSocketState;
>>
>>  typedef struct NetSocketListenState {
>> +    VLANClientState *nc;
>>      VLANState *vlan;
>>      char *model;
>>      char *name;
>> @@ -389,6 +390,11 @@ static void net_socket_accept(void *opaque)
>>              break;
>>          }
>>      }
>> +
>> +    if (s->nc) {
>> +        qemu_del_vlan_client(s->nc);
>> +    }
>> +
>>      s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1);
>
> This has a few issues:
>
> net_socket_fd_init() does not set the peer, only vlan.  This means the
> connected socket and NIC are not peered up.
>
> qemu_del_vlan_client() brings the link down...we never bring it back up.
>
> We need to avoid leaking s->nc because it is not freed in
> qemu_del_vlan_client().  Once peering is fixed s->nc will not be freed
> during NIC deletion anymore!
>
> It leaves a dangling pointer to s->nc in the qdev netdev property and
> NICInfo nd_table[].  Not sure if this is a problem but it's messy.
>
> I suggest using the real NetSocketState instead - do not create a dummy
> VLANClientState.  This means we create the socket fd NetSocketState
> right away and never need to update the peer.  Simply bring the link up
> once the socket file descriptor is connected.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu