Skip to contentSkip to conversation

net: fix sending of data with -net socket, listen backend

PATCH In review Daniel P. Berrange 2 participants 2 messages Nov 4, 2016 – Nov 7, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 net: fix sending of data with -net socket, listen backend

The use of -net socket,listen was broken in the following commit

commit 16a3df403b10c4ac347159e39005fd520b2648bb
Author: Zhang Chen <[email protected]>
Date: Fri May 13 15:35:19 2016 +0800

net/net: Add SocketReadState for reuse codes

This function is from net/socket.c, move it to net.c and net.h. Add SocketReadState to make others reuse net_fill_rstate(). suggestion from jason.

This refactored the state out of NetSocketState into a separate SocketReadState. This refactoring requires that a callback is provided to be triggered upon completion of a packet receive from the guest.

The patch only registered this callback in the codepaths hit by -net socket,connect, not -net socket,listen. So as a result packets sent by the guest in the latter case get dropped on the floor.

This bug is hidden because net_fill_rstate() silently does nothing if the callback is not set.

This patch adds in the middle callback registration and also adds an assert so that QEMU aborts if there are any other codepaths hit which are missing the callback.

---
 net/net.c    | 5 ++---
 net/socket.c | 1 +
 2 files changed, 3 insertions(+), 3 deletions(-)
net/net.c +2 -3
index ec984bf..939fe31 100644
@@ -1653,9 +1653,8 @@ int net_fill_rstate(SocketReadState *rs, const uint8_t *buf, int size)
             if (rs->index >= rs->packet_len) {
                 rs->index = 0;
                 rs->state = 0;
-                if (rs->finalize) {
-                    rs->finalize(rs);
-                }
+                assert(rs->finalize);
+                rs->finalize(rs);
             }
             break;
         }
net/socket.c +1 -0
index 982c8de..fe3547b 100644
@@ -511,6 +511,7 @@ static int net_socket_listen_init(NetClientState *peer,
     s->fd = -1;
     s->listen_fd = ret;
     s->nc.link_down = true;
+    net_socket_rs_init(&s->rs, net_socket_rs_finalize);
 
     qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
     qapi_free_SocketAddress(saddr);
Signature · 1 line
2.9.3
Reviewed-by on v1 1/1
#1 lore raw
Quoted 26 lines · Daniel P. Berrange

The use of -net socket,listen was broken in the following commit

commit 16a3df403b10c4ac347159e39005fd520b2648bb
Author: Zhang Chen <[email protected]>
Date: Fri May 13 15:35:19 2016 +0800

net/net: Add SocketReadState for reuse codes

This function is from net/socket.c, move it to net.c and net.h. Add SocketReadState to make others reuse net_fill_rstate(). suggestion from jason.

This refactored the state out of NetSocketState into a separate SocketReadState. This refactoring requires that a callback is provided to be triggered upon completion of a packet receive from the guest.

The patch only registered this callback in the codepaths hit by -net socket,connect, not -net socket,listen. So as a result packets sent by the guest in the latter case get dropped on the floor.

This bug is hidden because net_fill_rstate() silently does nothing if the callback is not set.

Quoted 6 lines

This patch adds in the middle callback registration and also adds an assert so that QEMU aborts if there are any other codepaths hit which are missing the callback.

Good catch!

Thanks
Zhang Chen

Quoted 33 lines
---
  net/net.c    | 5 ++---
  net/socket.c | 1 +
  2 files changed, 3 insertions(+), 3 deletions(-)
net/net.c +2 -3
index ec984bf..939fe31 100644
@@ -1653,9 +1653,8 @@ int net_fill_rstate(SocketReadState *rs, const uint8_t *buf, int size)
              if (rs->index >= rs->packet_len) {
                  rs->index = 0;
                  rs->state = 0;
-                if (rs->finalize) {
-                    rs->finalize(rs);
-                }
+                assert(rs->finalize);
+                rs->finalize(rs);
              }
              break;
          }
net/socket.c +1 -0
index 982c8de..fe3547b 100644
@@ -511,6 +511,7 @@ static int net_socket_listen_init(NetClientState *peer,
      s->fd = -1;
      s->listen_fd = ret;
      s->nc.link_down = true;
+    net_socket_rs_init(&s->rs, net_socket_rs_finalize);
 
      qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
      qapi_free_SocketAddress(saddr);
Signature · 2 lines
Thanks
zhangchen