net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other:

tcp_sendfile() reads data directly from a file and does not use NET_TCP_WRITE_BUFFERS data flow
even if CONFIG_NET_TCP_WRITE_BUFFERS option is enabled.
Despite this, tcp_sendfile relied on NET_TCP_WRITE_BUFFERS specific flow control variables that
were idle during sendfile operation. Thus it was a total inconsistency.

E.g. because of the issue, TCP socket used by sendfile() operation never issued
FIN packet on close() command, and the TCP connection hung up.

As a result of the fix, simultaneously enabled CONFIG_NET_TCP_WRITE_BUFFERS and
CONFIG_NET_SENDFILE options can coexist.
This commit is contained in:
Alexander Lunev 2022-01-16 07:40:08 +03:00 committed by Xiang Xiao
parent e97ba17451
commit 0f080cdeaf
4 changed files with 32 additions and 8 deletions

View File

@ -287,6 +287,10 @@ struct tcp_conn_s
uint8_t keepretries; /* Number of retries attempted */ uint8_t keepretries; /* Number of retries attempted */
#endif #endif
#if defined(CONFIG_NET_SENDFILE) && defined(CONFIG_NET_TCP_WRITE_BUFFERS)
bool sendfile; /* True if sendfile operation is in progress */
#endif
/* connevents is a list of callbacks for each socket the uses this /* connevents is a list of callbacks for each socket the uses this
* connection (there can be more that one in the event that the the socket * connection (there can be more that one in the event that the the socket
* was dup'ed). It is used with the network monitor to handle * was dup'ed). It is used with the network monitor to handle

View File

@ -226,9 +226,19 @@ void tcp_appsend(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
else else
{ {
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS /* The application cannot send more than what is allowed by the
* MSS (the minimum of the MSS and the available window).
*/
DEBUGASSERT(dev->d_sndlen <= conn->mss); DEBUGASSERT(dev->d_sndlen <= conn->mss);
#else
#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS) || defined(CONFIG_NET_SENDFILE)
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
if (conn->sendfile)
{
#endif
/* If d_sndlen > 0, the application has data to be sent. */ /* If d_sndlen > 0, the application has data to be sent. */
if (dev->d_sndlen > 0) if (dev->d_sndlen > 0)
@ -241,15 +251,14 @@ void tcp_appsend(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
*/ */
conn->tx_unacked += dev->d_sndlen; conn->tx_unacked += dev->d_sndlen;
/* The application cannot send more than what is allowed by the
* MSS (the minimum of the MSS and the available window).
*/
DEBUGASSERT(dev->d_sndlen <= conn->mss);
} }
conn->nrtx = 0; conn->nrtx = 0;
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
}
#endif
#endif #endif
/* Then handle the rest of the operation just as for the rexmit case */ /* Then handle the rest of the operation just as for the rexmit case */

View File

@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/ */
net_lock(); net_lock();
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
conn->sendfile = true;
#endif
memset(&state, 0, sizeof(struct sendfile_s)); memset(&state, 0, sizeof(struct sendfile_s));
/* This semaphore is used for signaling and, hence, should not have /* This semaphore is used for signaling and, hence, should not have
@ -574,6 +577,9 @@ errout_datacb:
errout_locked: errout_locked:
nxsem_destroy(&state.snd_sem); nxsem_destroy(&state.snd_sem);
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
conn->sendfile = false;
#endif
net_unlock(); net_unlock();
/* Return the current file position */ /* Return the current file position */

View File

@ -269,7 +269,12 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
else if ( else if (
#ifdef CONFIG_NET_TCP_WRITE_BUFFERS #ifdef CONFIG_NET_TCP_WRITE_BUFFERS
# ifdef CONFIG_NET_SENDFILE
(!conn->sendfile && conn->expired > 0) ||
(conn->sendfile && conn->nrtx >= TCP_MAXRTX) ||
# else
conn->expired > 0 || conn->expired > 0 ||
# endif
#else #else
conn->nrtx >= TCP_MAXRTX || conn->nrtx >= TCP_MAXRTX ||
#endif #endif