[PATCH 0/5] Fix cppcheck-2.21.0 warnings

[PATCH 0/5] Fix cppcheck-2.21.0 warnings

[David Gibson]

Another cppcheck version, another batch of false positives :(.  These
appears to be cppcheck regressions, I've filed two bugs on their
tracker.

Still, here are workarounds in the passt code.  At least I managed to
make a few genuine if minor improvements to the code along the way.

David Gibson (5):
  cppcheck: Remove unused CPPCHECK_6936
  cppcheck: Add workaround for cppcheck bug 14847
  tcp: Merge common sequence logic from tcp_{buf,vu}_data_from_sock()
  tcp: Avoid SEQ_*() comparisons against 0
  tcp: MAX_WINDOW should be unsigned

 Makefile       |  3 +--
 tcp.c          | 50 +++++++++++++++++++++++++++++++++++---------------
 tcp_buf.c      | 28 ++++------------------------
 tcp_buf.h      |  3 ++-
 tcp_internal.h |  4 ++--
 tcp_vu.c       | 27 ++++-----------------------
 tcp_vu.h       |  3 ++-
 util.c         |  6 ++++--
 8 files changed, 54 insertions(+), 70 deletions(-)

-- 
2.54.0

[PATCH 1/5] cppcheck: Remove unused CPPCHECK_6936

[David Gibson]

Our flags for cppcheck include -D CPPCHECK_6936.  This used to enable a
workaround for a cppcheck bug, but the code where we used it went away
in commit 9153aca15bc1.  Remove the now unused flag.

Fixes: 9153aca15bc1 ("util: Add abort_with_msg() and ASSERT_WITH_MSG() helpers")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 0a0a60b0..c19383f3 100644
--- a/Makefile
+++ b/Makefile
@@ -208,8 +208,7 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	else								\
 		echo "";						\
 	fi)								\
-	--suppress=missingIncludeSystem					\
-	 -D CPPCHECK_6936
+	--suppress=missingIncludeSystem
 
 cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
 
-- 
2.54.0

[PATCH 2/5] cppcheck: Add workaround for cppcheck bug 14847

[David Gibson]

It seems the function pointer argument confuses cppcheck 2.21.0 (at least),
causing it to think do_clone()'s definition has different argument names
than it's declaration, even though that's not the case.  I made a minimal
reproducer and filed a cppcheck bug for this (see link).  In the meanwhile
work around it with an explicit suppression.

Link: https://trac.cppcheck.net/ticket/14847
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index 43977d42..ed874f1c 100644
--- a/util.c
+++ b/util.c
@@ -713,8 +713,10 @@ int __clone2(int (*fn)(void *), void *stack_base, size_t stack_size, int flags,
  *
  * Return: thread ID of child, -1 on failure
  */
-int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
-	     void *arg)
+int do_clone(int (*fn)(void *),
+/* false positive, see https://trac.cppcheck.net/ticket/14847 */
+/* cppcheck-suppress [funcArgNamesDifferentUnnamed,unmatchedSuppression] */
+	     char *stack_area, size_t stack_size, int flags, void *arg)
 {
 #ifdef __ia64__
 	return __clone2(fn, stack_area + stack_size / 2, stack_size / 2,
-- 
2.54.0

[PATCH 3/5] tcp: Merge common sequence logic from tcp_{buf,vu}_data_from_sock()

[David Gibson]

tcp_buf_data_from_sock() and tcp_vu_data_from_sock() start with some
identical logic to compute the amount of data from the socket that has
already been sent to tap but not acked.  Replace it with a single copy in
their common caller tcp_data_from_sock(), passing the already_sent amount
in as a parameter.

This does mean that we now execute this logic in the VU case even if the
queue is not active.  The logic is still valid in this case, and we don't
need to worry too much about wasting cycles in this edge case.

Cc: Laurent Vivier <lvivier@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c     | 29 +++++++++++++++++++++++++++--
 tcp_buf.c | 28 ++++------------------------
 tcp_buf.h |  3 ++-
 tcp_vu.c  | 27 ++++-----------------------
 tcp_vu.h  |  3 ++-
 5 files changed, 39 insertions(+), 51 deletions(-)

diff --git a/tcp.c b/tcp.c
index c4000754..dcadc765 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1837,10 +1837,35 @@ static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq)
  */
 static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
 {
+	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
+	uint32_t already_sent;
+
+	/* How much have we read/sent since last received ack ? */
+	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
+
+	if (SEQ_LT(already_sent, 0)) {
+		/* RFC 761, section 2.1. */
+		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
+			   conn->seq_ack_from_tap, conn->seq_to_tap);
+		conn->seq_to_tap = conn->seq_ack_from_tap;
+		already_sent = 0;
+		if (tcp_set_peek_offset(conn, 0)) {
+			tcp_rst(c, conn);
+			return -1;
+		}
+	}
+
+	if (!wnd_scaled || already_sent >= wnd_scaled) {
+		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
+		conn_flag(c, conn, STALLED);
+		conn_flag(c, conn, ACK_FROM_TAP_DUE);
+		return 0;
+	}
+
 	if (c->mode == MODE_VU)
-		return tcp_vu_data_from_sock(c, conn);
+		return tcp_vu_data_from_sock(c, conn, already_sent);
 
-	return tcp_buf_data_from_sock(c, conn);
+	return tcp_buf_data_from_sock(c, conn, already_sent);
 }
 
 /**
diff --git a/tcp_buf.c b/tcp_buf.c
index ca356089..1fc49959 100644
--- a/tcp_buf.c
+++ b/tcp_buf.c
@@ -309,42 +309,22 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
  * tcp_buf_data_from_sock() - Handle new data from socket, queue to tap, in window
  * @c:		Execution context
  * @conn:	Connection pointer
+ * @already_sent:	Number of bytes already sent to tap, but not acked
  *
  * Return: negative on connection reset, 0 otherwise
  *
  * #syscalls recvmsg
  */
-int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
+int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
+			   uint32_t already_sent)
 {
 	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
 	int fill_bufs, send_bufs = 0, last_len, iov_rem = 0;
 	int len, dlen, i, s = conn->sock;
 	struct msghdr mh_sock = { 0 };
 	uint16_t mss = MSS_GET(conn);
-	uint32_t already_sent, seq;
 	struct iovec *iov;
-
-	/* How much have we read/sent since last received ack ? */
-	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
-
-	if (SEQ_LT(already_sent, 0)) {
-		/* RFC 761, section 2.1. */
-		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
-			   conn->seq_ack_from_tap, conn->seq_to_tap);
-		conn->seq_to_tap = conn->seq_ack_from_tap;
-		already_sent = 0;
-		if (tcp_set_peek_offset(conn, 0)) {
-			tcp_rst(c, conn);
-			return -1;
-		}
-	}
-
-	if (!wnd_scaled || already_sent >= wnd_scaled) {
-		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
-		conn_flag(c, conn, STALLED);
-		conn_flag(c, conn, ACK_FROM_TAP_DUE);
-		return 0;
-	}
+	uint32_t seq;
 
 	/* Set up buffer descriptors we'll fill completely and partially. */
 	fill_bufs = DIV_ROUND_UP(wnd_scaled - already_sent, mss);
diff --git a/tcp_buf.h b/tcp_buf.h
index 54f5e53f..710ed377 100644
--- a/tcp_buf.h
+++ b/tcp_buf.h
@@ -8,7 +8,8 @@
 
 void tcp_sock_iov_init(const struct ctx *c);
 void tcp_payload_flush(const struct ctx *c);
-int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn);
+int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
+			   uint32_t already_sent);
 int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags);
 
 #endif  /*TCP_BUF_H */
diff --git a/tcp_vu.c b/tcp_vu.c
index 7e2a7dbc..00616955 100644
--- a/tcp_vu.c
+++ b/tcp_vu.c
@@ -423,46 +423,27 @@ static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *conn,
  *			     in window
  * @c:		Execution context
  * @conn:	Connection pointer
+ * @already_sent:	Number of bytes already sent to tap, but not acked
  *
  * Return: negative on connection reset, 0 otherwise
  */
-int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
+int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
+			  uint32_t already_sent)
 {
 	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
 	struct vu_dev *vdev = c->vdev;
 	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
-	uint32_t already_sent, check;
 	ssize_t len, previous_dlen;
 	int i, elem_cnt, frame_cnt;
 	size_t hdrlen, fillsize;
 	int v6 = CONN_V6(conn);
+	uint32_t check;
 
 	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
 		debug("Got packet, but RX virtqueue not usable yet");
 		return 0;
 	}
 
-	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
-
-	if (SEQ_LT(already_sent, 0)) {
-		/* RFC 761, section 2.1. */
-		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
-			   conn->seq_ack_from_tap, conn->seq_to_tap);
-		conn->seq_to_tap = conn->seq_ack_from_tap;
-		already_sent = 0;
-		if (tcp_set_peek_offset(conn, 0)) {
-			tcp_rst(c, conn);
-			return -1;
-		}
-	}
-
-	if (!wnd_scaled || already_sent >= wnd_scaled) {
-		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
-		conn_flag(c, conn, STALLED);
-		conn_flag(c, conn, ACK_FROM_TAP_DUE);
-		return 0;
-	}
-
 	/* Set up buffer descriptors we'll fill completely and partially. */
 
 	fillsize = wnd_scaled - already_sent;
diff --git a/tcp_vu.h b/tcp_vu.h
index 6ab6057f..30e46925 100644
--- a/tcp_vu.h
+++ b/tcp_vu.h
@@ -7,6 +7,7 @@
 #define TCP_VU_H
 
 int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags);
-int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn);
+int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
+			  uint32_t already_sent);
 
 #endif  /*TCP_VU_H */
-- 
2.54.0

Re: [PATCH 3/5] tcp: Merge common sequence logic from tcp_{buf,vu}_data_from_sock()

[Laurent Vivier]

On 6/15/26 10:18, David Gibson wrote:
> tcp_buf_data_from_sock() and tcp_vu_data_from_sock() start with some
> identical logic to compute the amount of data from the socket that has
> already been sent to tap but not acked.  Replace it with a single copy in
> their common caller tcp_data_from_sock(), passing the already_sent amount
> in as a parameter.
> 
> This does mean that we now execute this logic in the VU case even if the
> queue is not active.  The logic is still valid in this case, and we don't
> need to worry too much about wasting cycles in this edge case.
> 
> Cc: Laurent Vivier <lvivier@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>   tcp.c     | 29 +++++++++++++++++++++++++++--
>   tcp_buf.c | 28 ++++------------------------
>   tcp_buf.h |  3 ++-
>   tcp_vu.c  | 27 ++++-----------------------
>   tcp_vu.h  |  3 ++-
>   5 files changed, 39 insertions(+), 51 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index c4000754..dcadc765 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1837,10 +1837,35 @@ static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq)
>    */
>   static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>   {
> +	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> +	uint32_t already_sent;
> +
> +	/* How much have we read/sent since last received ack ? */
> +	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> +
> +	if (SEQ_LT(already_sent, 0)) {
> +		/* RFC 761, section 2.1. */
> +		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
> +			   conn->seq_ack_from_tap, conn->seq_to_tap);
> +		conn->seq_to_tap = conn->seq_ack_from_tap;
> +		already_sent = 0;
> +		if (tcp_set_peek_offset(conn, 0)) {
> +			tcp_rst(c, conn);
> +			return -1;
> +		}
> +	}
> +
> +	if (!wnd_scaled || already_sent >= wnd_scaled) {
> +		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
> +		conn_flag(c, conn, STALLED);
> +		conn_flag(c, conn, ACK_FROM_TAP_DUE);
> +		return 0;
> +	}
> +
>   	if (c->mode == MODE_VU)
> -		return tcp_vu_data_from_sock(c, conn);
> +		return tcp_vu_data_from_sock(c, conn, already_sent);
>   
> -	return tcp_buf_data_from_sock(c, conn);
> +	return tcp_buf_data_from_sock(c, conn, already_sent);
>   }
>   
>   /**
> diff --git a/tcp_buf.c b/tcp_buf.c
> index ca356089..1fc49959 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -309,42 +309,22 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>    * tcp_buf_data_from_sock() - Handle new data from socket, queue to tap, in window
>    * @c:		Execution context
>    * @conn:	Connection pointer
> + * @already_sent:	Number of bytes already sent to tap, but not acked
>    *
>    * Return: negative on connection reset, 0 otherwise
>    *
>    * #syscalls recvmsg
>    */
> -int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> +int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
> +			   uint32_t already_sent)
>   {
>   	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
>   	int fill_bufs, send_bufs = 0, last_len, iov_rem = 0;
>   	int len, dlen, i, s = conn->sock;
>   	struct msghdr mh_sock = { 0 };
>   	uint16_t mss = MSS_GET(conn);
> -	uint32_t already_sent, seq;
>   	struct iovec *iov;
> -
> -	/* How much have we read/sent since last received ack ? */
> -	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> -
> -	if (SEQ_LT(already_sent, 0)) {
> -		/* RFC 761, section 2.1. */
> -		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
> -			   conn->seq_ack_from_tap, conn->seq_to_tap);
> -		conn->seq_to_tap = conn->seq_ack_from_tap;
> -		already_sent = 0;
> -		if (tcp_set_peek_offset(conn, 0)) {
> -			tcp_rst(c, conn);
> -			return -1;
> -		}
> -	}
> -
> -	if (!wnd_scaled || already_sent >= wnd_scaled) {
> -		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
> -		conn_flag(c, conn, STALLED);
> -		conn_flag(c, conn, ACK_FROM_TAP_DUE);
> -		return 0;
> -	}
> +	uint32_t seq;
>   
>   	/* Set up buffer descriptors we'll fill completely and partially. */
>   	fill_bufs = DIV_ROUND_UP(wnd_scaled - already_sent, mss);
> diff --git a/tcp_buf.h b/tcp_buf.h
> index 54f5e53f..710ed377 100644
> --- a/tcp_buf.h
> +++ b/tcp_buf.h
> @@ -8,7 +8,8 @@
>   
>   void tcp_sock_iov_init(const struct ctx *c);
>   void tcp_payload_flush(const struct ctx *c);
> -int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn);
> +int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
> +			   uint32_t already_sent);
>   int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags);
>   
>   #endif  /*TCP_BUF_H */
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 7e2a7dbc..00616955 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -423,46 +423,27 @@ static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *conn,
>    *			     in window
>    * @c:		Execution context
>    * @conn:	Connection pointer
> + * @already_sent:	Number of bytes already sent to tap, but not acked
>    *
>    * Return: negative on connection reset, 0 otherwise
>    */
> -int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> +int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
> +			  uint32_t already_sent)
>   {
>   	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
>   	struct vu_dev *vdev = c->vdev;
>   	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> -	uint32_t already_sent, check;
>   	ssize_t len, previous_dlen;
>   	int i, elem_cnt, frame_cnt;
>   	size_t hdrlen, fillsize;
>   	int v6 = CONN_V6(conn);
> +	uint32_t check;
>   
>   	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
>   		debug("Got packet, but RX virtqueue not usable yet");
>   		return 0;
>   	}
>   
> -	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> -
> -	if (SEQ_LT(already_sent, 0)) {
> -		/* RFC 761, section 2.1. */
> -		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
> -			   conn->seq_ack_from_tap, conn->seq_to_tap);
> -		conn->seq_to_tap = conn->seq_ack_from_tap;
> -		already_sent = 0;
> -		if (tcp_set_peek_offset(conn, 0)) {
> -			tcp_rst(c, conn);
> -			return -1;
> -		}
> -	}
> -
> -	if (!wnd_scaled || already_sent >= wnd_scaled) {
> -		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
> -		conn_flag(c, conn, STALLED);
> -		conn_flag(c, conn, ACK_FROM_TAP_DUE);
> -		return 0;
> -	}
> -
>   	/* Set up buffer descriptors we'll fill completely and partially. */
>   
>   	fillsize = wnd_scaled - already_sent;
> diff --git a/tcp_vu.h b/tcp_vu.h
> index 6ab6057f..30e46925 100644
> --- a/tcp_vu.h
> +++ b/tcp_vu.h
> @@ -7,6 +7,7 @@
>   #define TCP_VU_H
>   
>   int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags);
> -int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn);
> +int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
> +			  uint32_t already_sent);
>   
>   #endif  /*TCP_VU_H */

[PATCH 4/5] tcp: Avoid SEQ_*() comparisons against 0

[David Gibson]

Usually we use the SEQ_{LT,LE,GT,GE}() comparison macros on raw sequence
numbers, to see how they related handling possible wraparounds.  However,
in two cases we take a difference of sequence numbers then compare that
difference to 0.

As long as the difference is done using wrapping unsigned arithmetic,
that's not incorrect.  It's essentially doing comparisons in a "shifted"
space of sequence numbers, and I find working in both the raw and shifted
spaces makes it harder to reason about.  Specifically I sometimes find I
have to go through an additional layer of logic to convince myself that the
wraparound handling is correct.

In addition to confusing me, it also tends to confuse cppcheck, at least in
cppcheck 2.21.0 it often seems to trip false positives via cppcheck bug
14848.

It turns out that in each of the cases we use comparisons of sequence
differences against 0, it's pretty much just as clear - maybe clearer to
perform the comparisons against raw sequence numbers.  We still need the
difference afterwards, but it's much more obvious that it means what we
need it do, if we've already eliminated cases where the sequence numbers
aren't in the expected order.

In the case in tcp_data_from_tap() this also lets us simplify the diagram
explanation of the different cases a bit.  While there we correct the
diagram in the second discard case: seq was shown as partway into the
packet, which is not true by definition.  Instead seq_from_tap should be
some distance after the end of the packet.

Link: https://trac.cppcheck.net/ticket/14848
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/tcp.c b/tcp.c
index dcadc765..edcabe27 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1840,21 +1840,20 @@ static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
 	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
 	uint32_t already_sent;
 
-	/* How much have we read/sent since last received ack ? */
-	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
-
-	if (SEQ_LT(already_sent, 0)) {
+	if (SEQ_LT(conn->seq_to_tap, conn->seq_ack_from_tap)) {
 		/* RFC 761, section 2.1. */
 		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
 			   conn->seq_ack_from_tap, conn->seq_to_tap);
 		conn->seq_to_tap = conn->seq_ack_from_tap;
-		already_sent = 0;
 		if (tcp_set_peek_offset(conn, 0)) {
 			tcp_rst(c, conn);
 			return -1;
 		}
 	}
 
+	/* How much have we read/sent since last received ack ? */
+	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
+
 	if (!wnd_scaled || already_sent >= wnd_scaled) {
 		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
 		conn_flag(c, conn, STALLED);
@@ -1997,37 +1996,34 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
 		if (!len)
 			continue;
 
-		seq_offset = seq_from_tap - seq;
 		/* Use data from this buffer only in these two cases:
 		 *
 		 *      , seq_from_tap           , seq_from_tap
 		 * |--------| <-- len            |--------| <-- len
-		 * '----' <-- offset             ' <-- offset
 		 * ^ seq                         ^ seq
-		 *    (offset >= 0, seq + len > seq_from_tap)
+		 *    (seq_from_tap >= seq, seq + len > seq_from_tap)
 		 *
 		 * discard in these two cases:
-		 *          , seq_from_tap                , seq_from_tap
+		 *          , seq_from_tap                   , seq_from_tap
 		 * |--------| <-- len            |--------| <-- len
-		 * '--------' <-- offset            '-----| <- offset
-		 * ^ seq                            ^ seq
-		 *    (offset >= 0, seq + len <= seq_from_tap)
+		 * ^ seq                         ^ seq
+		 *    (seq_from_tap >= seq, seq + len <= seq_from_tap)
 		 *
 		 * keep, look for another buffer, then go back, in this case:
 		 *      , seq_from_tap
 		 *          |--------| <-- len
-		 *      '===' <-- offset
 		 *          ^ seq
-		 *    (offset < 0)
+		 *    (seq_from_tap < seq)
 		 */
-		if (SEQ_GE(seq_offset, 0) && SEQ_LE(seq + len, seq_from_tap))
+		if (SEQ_GE(seq_from_tap, seq) && SEQ_LE(seq + len, seq_from_tap))
 			continue;
 
-		if (SEQ_LT(seq_offset, 0)) {
+		if (SEQ_LT(seq_from_tap, seq)) {
 			if (keep == -1)
 				keep = i;
 			continue;
 		}
+		seq_offset = seq_from_tap - seq;
 
 		iov_drop_header(&data, seq_offset);
 		size = len - seq_offset;
-- 
2.54.0

[PATCH 5/5] tcp: MAX_WINDOW should be unsigned

[David Gibson]

Currently MAX_WINDOW is defined as a (signed) int, since '1' is a signed
literal.  This means that when it's used in the SEQ_*() macros, we're
making a signed / unsigned comparison.  While I don't think it was
incorrect in practice, confirming that requires reasoning though arcane
C type promotion rules.

Since it's obviously a non-negative value, change the constant to unsigned.
Change BUF_DISCARD_SIZE for the same reason, and because they're linked
via DISCARD_IOV_NUM, which must also be non-negative.  Change the types
of a few variables in tcp_prepare_iov() to match (otherwise we'd get some
compiler warnings).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c          | 3 +--
 tcp_internal.h | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/tcp.c b/tcp.c
index edcabe27..10d9d0d5 100644
--- a/tcp.c
+++ b/tcp.c
@@ -4069,9 +4069,8 @@ int tcp_prepare_iov(struct msghdr *msg, struct iovec *iov,
 		msg->msg_iov = iov + DISCARD_IOV_NUM;
 		msg->msg_iovlen = payload_iov_cnt;
 	} else {
-		int discard_cnt, discard_iov_rem;
+		unsigned discard_cnt, discard_iov_rem, i;
 		struct iovec *iov_start;
-		int i;
 
 		discard_cnt = DIV_ROUND_UP(already_sent, BUF_DISCARD_SIZE);
 		if (discard_cnt > DISCARD_IOV_NUM) {
diff --git a/tcp_internal.h b/tcp_internal.h
index 40472c99..c623569c 100644
--- a/tcp_internal.h
+++ b/tcp_internal.h
@@ -12,9 +12,9 @@
 #include "util.h"
 
 #define MAX_WS				8
-#define MAX_WINDOW			(1 << (16 + (MAX_WS)))
+#define MAX_WINDOW			(1U << (16 + (MAX_WS)))
 
-#define BUF_DISCARD_SIZE	(1 << 20)
+#define BUF_DISCARD_SIZE	(1U << 20)
 #define DISCARD_IOV_NUM		DIV_ROUND_UP(MAX_WINDOW, BUF_DISCARD_SIZE)
 
 #define MSS4				ROUND_DOWN(IP_MAX_MTU -		   \
-- 
2.54.0