zhhyu7 commented on code in PR #20627:
URL: https://github.com/apache/nuttx/pull/20627#discussion_r4283111996
##########
net/tcp/tcp_input.c:
##########
@@ -1570,38 +1628,72 @@ static void tcp_input(FAR struct net_driver_s *dev,
uint8_t domain,
if ((tcp->flags & TCP_URG) != 0)
{
- dev->d_urglen = (tcp->urgp[0] << 8) | tcp->urgp[1];
- if (dev->d_urglen > dev->d_len)
+ uint16_t urgp = (tcp->urgp[0] << 8) | tcp->urgp[1];
Review Comment:
`TCP_FIN_WAIT_1` and `TCP_FIN_WAIT_2` also accept data, but `URG` is ignored
in those states. Segments queued by `tcp_input_ofosegs()` lose `URG` as well,
and so do segments delivered later by `tcp_ofoseg_data_event()`
(`net/tcp/tcp_callback.c:112`). If the segment that carries the urgent byte
arrives out of order, the state can stay at `TCP_URG_AWAITING` for good.
Please move the logic into a helper, for example `tcp_urg_input(conn, tcp,
seq, len)`. Call it from every path that accepts data.
And with an absolute `rcv_up`, reassembly of out-of-order segments needs no
extra handling.
##########
net/tcp/tcp.h:
##########
@@ -297,6 +306,14 @@ struct tcp_conn_s
FAR struct iob_s *readahead; /* Read-ahead buffering */
+#ifdef CONFIG_NET_TCPURGDATA
+ /* Urgent (out-of-band) data handling */
+
+ uint8_t urg_data; /* Received urgent byte */
+ uint8_t urg_state; /* Urgent data state (TCP_URG_*) */
+ uint16_t urg_offset; /* Outgoing urgent data offset (0 if none) */
Review Comment:
```conn->urg_offset``` and ```wrb->wb_urg``` are offsets relative to a
segment and to a write buffer. Please replace them with one absolute sequence
number per connection, for example:
```
uint32_t snd_wrseq; /* The sequence number of the next byte to be added to
the write queue */
uint32_t snd_up; /* Sequence number one past the urgent byte */
```
These places exist only to keep wb_urg / urg_offset consistent:
- the adjustment in the two TCP_WBTRIM() paths;
- the conversion at first transmission and at fast retransmission;
- the exception that prevents coalescing;
- the initialisation and the assignment.
Please remove all of them and maintain only ```snd_up``` instead. Set
`snd_up` and `snd_urgmode` once in `psock_tcp_send()`, when the last byte of a
`MSG_OOB` buffer is queued. After that, the urgent pointer of each segment is
derived from `snd_up` in `tcp_sendcommon()` (see below). Coalescing no longer
needs to be restricted, and retransmission, trimming and chunking need no
urgent-specific code.
##########
net/tcp/tcp_send_unbuffered.c:
##########
@@ -99,6 +99,9 @@ struct send_s
#endif
int snd_dup_acks; /* Duplicate ACK counter */
#endif
+#ifdef CONFIG_NET_TCPURGDATA
+ int snd_flags; /* Send flags (e.g., MSG_OOB) */
Review Comment:
Please remove these as well: `snd_flags` and the per-segment `urg_offset =
sndlen`. Instead, set `snd_up` once in `psock_tcp_send()`, from `snd_isn` and
`snd_buflen`. This also fixes the "one urgent mark per segment" issue.
##########
net/tcp/tcp_send.c:
##########
@@ -174,8 +174,24 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
tcp->wnd[1] = recvwndo & 0xff;
}
+#ifdef CONFIG_NET_TCPURGDATA
+ /* If caller explicitly asked for URG, set the urgent pointer from conn */
+
+ if ((tcp->flags & TCP_URG) != 0 && conn->urg_offset > 0)
Review Comment:
`tcp_sendcommon()` currently reads `conn->urg_offset` and also clears it. A
header builder should not change connection state, and with the current code
the result depends on `tcp_rexmit()` having set `TCP_URG` beforehand. With
`snd_up`, the URG fields can be computed for every outgoing segment from its
sequence number alone:
```c
if (TCP_SEQ_GT(conn->snd_up, seq) &&
TCP_SEQ_SUB(conn->snd_up, seq) <= 0xffff)
{
uint32_t up = TCP_SEQ_SUB(conn->snd_up, seq);
tcp->flags |= TCP_URG;
tcp->urgp[0] = (up > 0xffff ? 0xffff : up) >> 8;
tcp->urgp[1] = (up > 0xffff ? 0xffff : up) & 0xff;
}
```
##########
net/tcp/tcp_appsend.c:
##########
@@ -277,6 +277,9 @@ void tcp_rexmit(FAR struct net_driver_s *dev, FAR struct
tcp_conn_s *conn,
uint16_t result)
{
uint8_t hdrlen;
+#ifdef CONFIG_NET_TCPURGDATA
+ uint16_t sendflags;
Review Comment:
Once `tcp_sendcommon()` sets `TCP_URG` itself, please remove `sendflags` and
the duplicated `#ifdef CONFIG_NET_TCPURGDATA / #else` calls to `tcp_send()`, so
that `tcp_rexmit()` returns to its original form. This also prevents a leftover
`urg_offset` from being attached to unrelated segments, such as the sendfile
path, which goes through the same code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
