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

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

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

As you have known, QEMU upstream currently doesn't support for -netdev socket,listen; This patch makes it work now.

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 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);
     if (s1) {
         snprintf(s1->nc.info_str, sizeof(s1->nc.info_str),
@@ -397,6 +403,13 @@ static void net_socket_accept(void *opaque)
     }
 }
 
+static NetClientInfo net_listen_info = {
+    .type = NET_CLIENT_TYPE_NONE,
+    .size = sizeof(VLANClientState),
+    .receive = NULL,
+    .cleanup = NULL,
+};
+
 static int net_socket_listen_init(VLANState *vlan,
                                   const char *model,
                                   const char *name,
@@ -441,6 +454,10 @@ static int net_socket_listen_init(VLANState *vlan,
     s->model = g_strdup(model);
     s->name = name ? g_strdup(name) : NULL;
     s->fd = fd;
+
+    s->nc = qemu_new_net_client(&net_listen_info, NULL, NULL, model, name);
+    s->nc->link_down = true;
+
     qemu_set_fd_handler(fd, net_socket_accept, NULL, s);
     return 0;
 }
-- 
1.7.6

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

[Stefan Hajnoczi]

On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@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
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 <wuzhy@linux.vnet.ibm.com>
> ---
>  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

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
<stefanha@linux.vnet.ibm.com> wrote:
> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@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.
>
> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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.
Ditto.
>
> 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.

>
> 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?
> 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.
good catch, the dummy VLANClientState will occupy one card slot. We
should free it before real VLANClientState is created.
>
> 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()"?
If we directly create one real NetSocketState, the code change will be
a bit large, right?
> right away and never need to update the peer.  Simply bring the link up
I think that backends never set their peer.
> once the socket file descriptor is connected.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu

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 <zwu.kernel@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>>> From: Zhi Yong Wu <wuzhy@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.

Stefan

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 <stefanha@gmail.com> wrote:
> On Sat, Feb 18, 2012 at 5:35 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>>>> From: Zhi Yong Wu <wuzhy@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

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
<stefanha@linux.vnet.ibm.com> wrote:
> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@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
> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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

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

[Stefan Hajnoczi]

On Sat, Feb 18, 2012 at 8:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>>> From: Zhi Yong Wu <wuzhy@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
>> 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?

s->nc.info_str is set to "socket: ...".  For
net_socket_fd_init_stream() you will have "socket: fd=%d".  The
info_str is displayed by "info network".

Stefan

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

[Zhi Yong Wu]

On Sat, Feb 18, 2012 at 5:52 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Feb 18, 2012 at 8:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Fri, Feb 17, 2012 at 6:24 PM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> On Fri, Feb 17, 2012 at 12:20:08PM +0800, zwu.kernel@gmail.com wrote:
>>>> From: Zhi Yong Wu <wuzhy@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
>>> 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?
>
> s->nc.info_str is set to "socket: ...".  For
> net_socket_fd_init_stream() you will have "socket: fd=%d".  The
This fd is displayed only for udp type. But in your test command, i
don't see that you specifiy if it is udp or tcp type.
> info_str is displayed by "info network".
>
> Stefan



-- 
Regards,

Zhi Yong Wu