diff mbox series

[v6,08/12] net/tcp: simplify tcp header filling code

Message ID 20240909222720.2563118-9-mikhail.kshevetskiy@iopsys.eu
State Superseded
Delegated to: Ramon Fried
Headers show
Series net: tcp: improve tcp support | expand

Commit Message

Mikhail Kshevetskiy Sept. 9, 2024, 10:27 p.m. UTC 
This patch:
 * remove useless code,
 * use a special function for pretty printing of tcp flags,
 * simplify the code

The behavior should not be changed.

Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 net/tcp.c | 74 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

Comments

Simon Glass Sept. 12, 2024, 12:59 a.m. UTC | #1 
Hi Mikhail,

On Mon, 9 Sept 2024 at 16:27, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> This patch:
>  * remove useless code,
>  * use a special function for pretty printing of tcp flags,
>  * simplify the code
>
> The behavior should not be changed.
>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  net/tcp.c | 74 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/net/tcp.c b/net/tcp.c
> index 37dda6f84f9..0778c153aca 100644
> --- a/net/tcp.c
> +++ b/net/tcp.c
> @@ -535,10 +535,41 @@ void net_set_syn_options(struct tcp_stream *tcp, union tcp_build_pkt *b)
>         b->ip.end = TCP_O_END;
>  }
>
> +const char *tcpflags_to_str(char tcpflags, char *buf, int size)
> +{
> +       int i, len;
> +       char *orig = buf;
> +       const struct {

static

> +               int             bit;
> +               const char      *name;
> +       } desc[] = {{TCP_RST, "RST"}, {TCP_SYN, "SYN"}, {TCP_PUSH, "PSH"},
> +                   {TCP_FIN, "FIN"}, {TCP_ACK, "ACK"}, {0, NULL}};
> +
> +       *orig = '\0';
> +       for (i = 0; desc[i].name; i++) {

You could use ARRAY_SIZE() here and avoid the terminator, if you like

> +               if (!(tcpflags & desc[i].bit))
> +                       continue;
> +
> +               len = strlen(desc[i].name);
> +               if (size <= len + 1)
> +                       break;
> +               if (buf != orig) {
> +                       *buf++ = ',';
> +                       size--;
> +               }
> +
> +               strcpy(buf, desc[i].name);

Or be lazy and just use strlcat() for both the command the name...up to you

> +               buf += len;
> +               size -= len;
> +       }

blank line before final return

> +       return orig;
> +}
> +
>  int tcp_set_tcp_header(struct tcp_stream *tcp, uchar *pkt, int payload_len,
>                        u8 action, u32 tcp_seq_num, u32 tcp_ack_num)
>  {
>         union tcp_build_pkt *b = (union tcp_build_pkt *)pkt;
> +       char buf[24];
>         int pkt_hdr_len;
>         int pkt_len;
>         int tcp_len;
> @@ -548,55 +579,32 @@ int tcp_set_tcp_header(struct tcp_stream *tcp, uchar *pkt, int payload_len,
>          *         4 bits reserved options
>          */
>         b->ip.hdr.tcp_flags = action;
> -       pkt_hdr_len = IP_TCP_HDR_SIZE;
>         b->ip.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>
>         switch (action) {
>         case TCP_SYN:
>                 debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:SYN (%pI4, %pI4, sq=%u, ak=%u)\n",
> -                          &tcp->rhost, &net_ip,
> -                          tcp_seq_num, tcp_ack_num);
> +                          "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
> +                          tcpflags_to_str(action, buf, sizeof(buf)),
> +                          &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
>                 net_set_syn_options(tcp, b);
>                 pkt_hdr_len = IP_TCP_O_SIZE;
>                 break;
> -       case TCP_SYN | TCP_ACK:
> -       case TCP_ACK:
> -               pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(tcp, b);
> -               b->ip.hdr.tcp_flags = action;
> -               debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:ACK (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
> -                          &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num,
> -                          action);
> -               break;
> -       case TCP_FIN:
> -               debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:FIN  (%pI4, %pI4, s=%u, a=%u)\n",
> -                          &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
> -               payload_len = 0;
> -               pkt_hdr_len = IP_TCP_HDR_SIZE;
> -               break;
>         case TCP_RST | TCP_ACK:
>         case TCP_RST:
>                 debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:RST  (%pI4, %pI4, s=%u, a=%u)\n",
> +                          "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
> +                          tcpflags_to_str(action, buf, sizeof(buf)),
>                            &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
> +               pkt_hdr_len = IP_TCP_HDR_SIZE;
>                 break;
> -       /* Notify connection closing */
> -       case (TCP_FIN | TCP_ACK):
> -       case (TCP_FIN | TCP_ACK | TCP_PUSH):
> -               debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:FIN ACK PSH(%pI4, %pI4, s=%u, a=%u, A=%x)\n",
> -                          &tcp->rhost, &net_ip,
> -                          tcp_seq_num, tcp_ack_num, action);
> -               fallthrough;
>         default:
>                 pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(tcp, b);
> -               b->ip.hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
>                 debug_cond(DEBUG_DEV_PKT,
> -                          "TCP Hdr:dft  (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
> -                          &tcp->rhost, &net_ip,
> -                          tcp_seq_num, tcp_ack_num, action);
> +                          "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
> +                          tcpflags_to_str(action, buf, sizeof(buf)),
> +                          &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
> +               break;
>         }
>
>         pkt_len = pkt_hdr_len + payload_len;
> --
> 2.45.2
>

Regards,
Simon
diff mbox series

Patch

diff --git a/net/tcp.c b/net/tcp.c
index 37dda6f84f9..0778c153aca 100644
--- a/net/tcp.c
+++ b/net/tcp.c
@@ -535,10 +535,41 @@  void net_set_syn_options(struct tcp_stream *tcp, union tcp_build_pkt *b)
 	b->ip.end = TCP_O_END;
 }
 
+const char *tcpflags_to_str(char tcpflags, char *buf, int size)
+{
+	int i, len;
+	char *orig = buf;
+	const struct {
+		int		bit;
+		const char	*name;
+	} desc[] = {{TCP_RST, "RST"}, {TCP_SYN, "SYN"}, {TCP_PUSH, "PSH"},
+		    {TCP_FIN, "FIN"}, {TCP_ACK, "ACK"}, {0, NULL}};
+
+	*orig = '\0';
+	for (i = 0; desc[i].name; i++) {
+		if (!(tcpflags & desc[i].bit))
+			continue;
+
+		len = strlen(desc[i].name);
+		if (size <= len + 1)
+			break;
+		if (buf != orig) {
+			*buf++ = ',';
+			size--;
+		}
+
+		strcpy(buf, desc[i].name);
+		buf += len;
+		size -= len;
+	}
+	return orig;
+}
+
 int tcp_set_tcp_header(struct tcp_stream *tcp, uchar *pkt, int payload_len,
 		       u8 action, u32 tcp_seq_num, u32 tcp_ack_num)
 {
 	union tcp_build_pkt *b = (union tcp_build_pkt *)pkt;
+	char buf[24];
 	int pkt_hdr_len;
 	int pkt_len;
 	int tcp_len;
@@ -548,55 +579,32 @@  int tcp_set_tcp_header(struct tcp_stream *tcp, uchar *pkt, int payload_len,
 	 *         4 bits reserved options
 	 */
 	b->ip.hdr.tcp_flags = action;
-	pkt_hdr_len = IP_TCP_HDR_SIZE;
 	b->ip.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
 
 	switch (action) {
 	case TCP_SYN:
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:SYN (%pI4, %pI4, sq=%u, ak=%u)\n",
-			   &tcp->rhost, &net_ip,
-			   tcp_seq_num, tcp_ack_num);
+			   "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
+			   tcpflags_to_str(action, buf, sizeof(buf)),
+			   &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
 		net_set_syn_options(tcp, b);
 		pkt_hdr_len = IP_TCP_O_SIZE;
 		break;
-	case TCP_SYN | TCP_ACK:
-	case TCP_ACK:
-		pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(tcp, b);
-		b->ip.hdr.tcp_flags = action;
-		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:ACK (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
-			   &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num,
-			   action);
-		break;
-	case TCP_FIN:
-		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:FIN  (%pI4, %pI4, s=%u, a=%u)\n",
-			   &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
-		payload_len = 0;
-		pkt_hdr_len = IP_TCP_HDR_SIZE;
-		break;
 	case TCP_RST | TCP_ACK:
 	case TCP_RST:
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:RST  (%pI4, %pI4, s=%u, a=%u)\n",
+			   "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
+			   tcpflags_to_str(action, buf, sizeof(buf)),
 			   &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
+		pkt_hdr_len = IP_TCP_HDR_SIZE;
 		break;
-	/* Notify connection closing */
-	case (TCP_FIN | TCP_ACK):
-	case (TCP_FIN | TCP_ACK | TCP_PUSH):
-		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:FIN ACK PSH(%pI4, %pI4, s=%u, a=%u, A=%x)\n",
-			   &tcp->rhost, &net_ip,
-			   tcp_seq_num, tcp_ack_num, action);
-		fallthrough;
 	default:
 		pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(tcp, b);
-		b->ip.hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:dft  (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
-			   &tcp->rhost, &net_ip,
-			   tcp_seq_num, tcp_ack_num, action);
+			   "TCP Hdr:%s (%pI4, %pI4, s=%u, a=%u)\n",
+			   tcpflags_to_str(action, buf, sizeof(buf)),
+			   &tcp->rhost, &net_ip, tcp_seq_num, tcp_ack_num);
+		break;
 	}
 
 	pkt_len	= pkt_hdr_len + payload_len;