diff mbox series

[RESEND,v3,7/9] net/tcp: simplify tcp header filling code

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

Commit Message

Mikhail Kshevetskiy Aug. 14, 2024, 10:31 a.m. UTC 
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

Comments

Simon Glass Aug. 17, 2024, 3:58 p.m. UTC | #1 
Hi Mikhail,

On Wed, 14 Aug 2024 at 04:32, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

nits below

> diff --git a/net/tcp.c b/net/tcp.c
> index 2c34556c26d..7014d5b4f43 100644
> --- a/net/tcp.c
> +++ b/net/tcp.c
> @@ -527,10 +527,37 @@ 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 = 0, 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';
> +       while (desc[i].name != NULL) {

try to avoid comparing with NULL or 0

> +               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;
> @@ -540,55 +567,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.39.2
>

Regards,
Simon
Mikhail Kshevetskiy Aug. 23, 2024, 3:12 p.m. UTC | #2 
By the way, I'd like to know your opinion:
 * should I add a sample implementation of http/1.1 compatible
web-server to this patch series?

This can be used as a base for other implementations like firmware
upgrade web-server used by some vendors.

Mikhail Kshevetskiy


On 17.08.2024 18:58, Simon Glass wrote:
> Hi Mikhail,
>
> On Wed, 14 Aug 2024 at 04:32, Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> ---
>>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 37 insertions(+), 33 deletions(-)
>>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> nits below
>
>> diff --git a/net/tcp.c b/net/tcp.c
>> index 2c34556c26d..7014d5b4f43 100644
>> --- a/net/tcp.c
>> +++ b/net/tcp.c
>> @@ -527,10 +527,37 @@ 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 = 0, 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';
>> +       while (desc[i].name != NULL) {
> try to avoid comparing with NULL or 0
>
>> +               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;
>> @@ -540,55 +567,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.39.2
>>
> Regards,
> Simon
Simon Glass Aug. 23, 2024, 8:47 p.m. UTC | #3 
Hi Mikhail,

On Fri, 23 Aug 2024 at 09:12, Mikhail Kshevetskiy
<mikhail.kshevetskiy@genexis.eu> wrote:
>
> By the way, I'd like to know your opinion:
>  * should I add a sample implementation of http/1.1 compatible
> web-server to this patch series?
>
> This can be used as a base for other implementations like firmware
> upgrade web-server used by some vendors.

Do you mean using the sandbox emulator? Yes I think that would be very helpful.

Regards,
Simon


>
> Mikhail Kshevetskiy
>
>
> On 17.08.2024 18:58, Simon Glass wrote:
> > Hi Mikhail,
> >
> > On Wed, 14 Aug 2024 at 04:32, Mikhail Kshevetskiy
> > <mikhail.kshevetskiy@iopsys.eu> wrote:
> >> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> >> ---
> >>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
> >>  1 file changed, 37 insertions(+), 33 deletions(-)
> >>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > nits below
> >
> >> diff --git a/net/tcp.c b/net/tcp.c
> >> index 2c34556c26d..7014d5b4f43 100644
> >> --- a/net/tcp.c
> >> +++ b/net/tcp.c
> >> @@ -527,10 +527,37 @@ 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 = 0, 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';
> >> +       while (desc[i].name != NULL) {
> > try to avoid comparing with NULL or 0
> >
> >> +               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;
> >> @@ -540,55 +567,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.39.2
> >>
> > Regards,
> > Simon
Mikhail Kshevetskiy Aug. 24, 2024, 12:30 p.m. UTC | #4 
actually I know nothing about sandbox emulator


On 23.08.2024 23:47, Simon Glass wrote:
> Hi Mikhail,
>
> On Fri, 23 Aug 2024 at 09:12, Mikhail Kshevetskiy
> <mikhail.kshevetskiy@genexis.eu> wrote:
>> By the way, I'd like to know your opinion:
>>  * should I add a sample implementation of http/1.1 compatible
>> web-server to this patch series?
>>
>> This can be used as a base for other implementations like firmware
>> upgrade web-server used by some vendors.
> Do you mean using the sandbox emulator? Yes I think that would be very helpful.
>
> Regards,
> Simon
>
>
>> Mikhail Kshevetskiy
>>
>>
>> On 17.08.2024 18:58, Simon Glass wrote:
>>> Hi Mikhail,
>>>
>>> On Wed, 14 Aug 2024 at 04:32, Mikhail Kshevetskiy
>>> <mikhail.kshevetskiy@iopsys.eu> wrote:
>>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>>>> ---
>>>>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
>>>>  1 file changed, 37 insertions(+), 33 deletions(-)
>>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> nits below
>>>
>>>> diff --git a/net/tcp.c b/net/tcp.c
>>>> index 2c34556c26d..7014d5b4f43 100644
>>>> --- a/net/tcp.c
>>>> +++ b/net/tcp.c
>>>> @@ -527,10 +527,37 @@ 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 = 0, 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';
>>>> +       while (desc[i].name != NULL) {
>>> try to avoid comparing with NULL or 0
>>>
>>>> +               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;
>>>> @@ -540,55 +567,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.39.2
>>>>
>>> Regards,
>>> Simon
Simon Glass Aug. 26, 2024, 5:58 p.m. UTC | #5 
Hi Mikhail,

On Sat, 24 Aug 2024 at 06:30, Mikhail Kshevetskiy
<mikhail.kshevetskiy@genexis.eu> wrote:
>
> actually I know nothing about sandbox emulator

This is the foundation of U-Boot's testing [1]. See for example test/cmd/wget.c

>
>
> On 23.08.2024 23:47, Simon Glass wrote:
> > Hi Mikhail,
> >
> > On Fri, 23 Aug 2024 at 09:12, Mikhail Kshevetskiy
> > <mikhail.kshevetskiy@genexis.eu> wrote:
> >> By the way, I'd like to know your opinion:
> >>  * should I add a sample implementation of http/1.1 compatible
> >> web-server to this patch series?
> >>
> >> This can be used as a base for other implementations like firmware
> >> upgrade web-server used by some vendors.
> > Do you mean using the sandbox emulator? Yes I think that would be very helpful.
> >
> > Regards,
> > Simon
> >
> >
> >> Mikhail Kshevetskiy
> >>
> >>
> >> On 17.08.2024 18:58, Simon Glass wrote:
> >>> Hi Mikhail,
> >>>
> >>> On Wed, 14 Aug 2024 at 04:32, Mikhail Kshevetskiy
> >>> <mikhail.kshevetskiy@iopsys.eu> wrote:
> >>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> >>>> ---
> >>>>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
> >>>>  1 file changed, 37 insertions(+), 33 deletions(-)
> >>>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>

[..]

Regards,
Simon

[1] https://docs.u-boot.org/en/latest/develop/testing.html
Peter Robinson Aug. 28, 2024, 10:04 a.m. UTC | #6 
This needs an actual description, please add details what and why for
this 70 line patch.

> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  net/tcp.c | 70 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
>
> diff --git a/net/tcp.c b/net/tcp.c
> index 2c34556c26d..7014d5b4f43 100644
> --- a/net/tcp.c
> +++ b/net/tcp.c
> @@ -527,10 +527,37 @@ 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 = 0, 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';
> +       while (desc[i].name != NULL) {
> +               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;
> @@ -540,55 +567,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.39.2
>
diff mbox series

Patch

diff --git a/net/tcp.c b/net/tcp.c
index 2c34556c26d..7014d5b4f43 100644
--- a/net/tcp.c
+++ b/net/tcp.c
@@ -527,10 +527,37 @@  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 = 0, 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';
+	while (desc[i].name != NULL) {
+		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;
@@ -540,55 +567,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;