[ipxe-devel] [PATCH 1/4] [tcp] Deliver data only after updating TCP state

[Guo-Fu Tseng]

From: Guo-Fu Tseng <cooldavid at cooldavid.org>

Michael suggested something like:
===============================================================
  struct list_head received = LIST_HEAD_INIT ( received );
  ...
  while ( ! list_empty ( &tcp->rx_queue ) ) {
     ...
     tcp_rx_data ( tcp, seq, iob_disown ( iobuf ), &received );
     ...
  }

  list_for_each_entry_safe ( iobuf, tmp, &received, list ) {
    // deliver iobuf via xfer_deliver_iob()
  }

  if ( tcp->state & TCP_STATE_RCVD ( TCP_FIN ) )
    tcp_close ( tcp, 0 );
===============================================================
But after some thought I think making the change like this commit
can have same behavior, but simplified by not using extra queue.
Which can also save some code size.

In this patch:
  1. We call xfer_deliver_iob() after fully updated/handled received TCP
     state. In which to have better timing that upper-layer protocol
     might call tcp_xfer_close() to initiate a shutdown, or sending
     extra data.
  2. We move tcp_close() out from tcp_rx_fin() because tcp_close()
     nullified the xfer interface. It'll cause error to call
     xfer_deliver_iob() after tcp_rx_fin().
     (So that I proposed an idea to clean up tcp_close(), I'll post it
      in later commit.)

Signed-off-by: Guo-Fu Tseng <cooldavid at cooldavid.org>
---
 src/net/tcp.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/net/tcp.c b/src/net/tcp.c
index b2d8827..19815fb 100644
--- a/src/net/tcp.c
+++ b/src/net/tcp.c
@@ -861,18 +861,17 @@ static int tcp_rx_ack ( struct tcp_connection *tcp, uint32_t ack,
  *
  * This function takes ownership of the I/O buffer.
  */
-static int tcp_rx_data ( struct tcp_connection *tcp, uint32_t seq,
+static struct io_buffer* tcp_rx_data ( struct tcp_connection *tcp, uint32_t seq,
 			 struct io_buffer *iobuf ) {
 	uint32_t already_rcvd;
 	uint32_t len;
-	int rc;
 
 	/* Ignore duplicate or out-of-order data */
 	already_rcvd = ( tcp->rcv_ack - seq );
 	len = iob_len ( iobuf );
 	if ( already_rcvd >= len ) {
 		free_iob ( iobuf );
-		return 0;
+		return NULL;
 	}
 	iob_pull ( iobuf, already_rcvd );
 	len -= already_rcvd;
@@ -880,14 +879,7 @@ static int tcp_rx_data ( struct tcp_connection *tcp, uint32_t seq,
 	/* Acknowledge new data */
 	tcp_rx_seq ( tcp, len );
 
-	/* Deliver data to application */
-	if ( ( rc = xfer_deliver_iob ( &tcp->xfer, iobuf ) ) != 0 ) {
-		DBGC ( tcp, "TCP %p could not deliver %08x..%08x: %s\n",
-		       tcp, seq, ( seq + len ), strerror ( rc ) );
-		return rc;
-	}
-
-	return 0;
+	return iobuf;
 }
 
 /**
@@ -909,9 +901,6 @@ static int tcp_rx_fin ( struct tcp_connection *tcp, uint32_t seq ) {
 	/* Mark FIN as received */
 	tcp->tcp_state |= TCP_STATE_RCVD ( TCP_FIN );
 
-	/* Close connection */
-	tcp_close ( tcp, 0 );
-
 	return 0;
 }
 
@@ -1008,6 +997,7 @@ static void tcp_process_rx_queue ( struct tcp_connection *tcp ) {
 	uint32_t seq;
 	unsigned int flags;
 	size_t len;
+	int rc;
 
 	/* Process all applicable received buffers.  Note that we
 	 * cannot use list_for_each_entry() to iterate over the RX
@@ -1031,7 +1021,7 @@ static void tcp_process_rx_queue ( struct tcp_connection *tcp ) {
 		len = iob_len ( iobuf );
 
 		/* Handle new data, if any */
-		tcp_rx_data ( tcp, seq, iob_disown ( iobuf ) );
+		iobuf = tcp_rx_data ( tcp, seq, iobuf );
 		seq += len;
 
 		/* Handle FIN, if present */
@@ -1039,6 +1029,20 @@ static void tcp_process_rx_queue ( struct tcp_connection *tcp ) {
 			tcp_rx_fin ( tcp, seq );
 			seq++;
 		}
+
+		/* Deliver data to application, if any */
+		if ( iobuf &&
+		     ( rc = xfer_deliver_iob ( &tcp->xfer,
+					       iob_disown ( iobuf ) ) ) != 0 ) {
+			DBGC ( tcp, "TCP %p could not deliver %08x..%08x: %s\n",
+			       tcp, ( seq - len - ( flags & TCP_FIN ) ? 1 : 0 ),
+			       seq, strerror ( rc ) );
+		}
+	}
+
+	if ( tcp->tcp_state & TCP_STATE_RCVD ( TCP_FIN ) ) {
+		/* Close connection */
+		tcp_close ( tcp, 0 );
 	}
 }
 
-- 
1.7.1