| Message ID | 1329452408-13526-1-git-send-email-zwu.kernel@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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 >
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
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
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
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
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; }