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

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