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

[Stefan Hajnoczi]

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.

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