[incubator-nuttx] branch master updated: 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.

[xi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

xiaoxiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git


The following commit(s) were added to refs/heads/master by this push:
     new 0f080cd  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.
0f080cd is described below

commit 0f080cdeaf206253319256482a5cab850473d505
Author: Alexander Lunev <al...@mail.ru>
AuthorDate: Sun Jan 16 07:40:08 2022 +0300

    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.
---
 net/tcp/tcp.h          |  4 ++++
 net/tcp/tcp_appsend.c  | 23 ++++++++++++++++-------
 net/tcp/tcp_sendfile.c |  6 ++++++
 net/tcp/tcp_timer.c    |  5 +++++
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/net/tcp/tcp.h b/net/tcp/tcp.h
index 273db14..3992d6b 100644
--- a/net/tcp/tcp.h
+++ b/net/tcp/tcp.h
@@ -287,6 +287,10 @@ struct tcp_conn_s
   uint8_t    keepretries; /* Number of retries attempted */
 #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
    * 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
diff --git a/net/tcp/tcp_appsend.c b/net/tcp/tcp_appsend.c
index 467d567..82c7c9f 100644
--- a/net/tcp/tcp_appsend.c
+++ b/net/tcp/tcp_appsend.c
@@ -226,9 +226,19 @@ void tcp_appsend(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
 
   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);
-#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 (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;
+        }
 
-          /* The application cannot send more than what is allowed by the
-           * MSS (the minimum of the MSS and the available window).
-           */
+      conn->nrtx = 0;
 
-          DEBUGASSERT(dev->d_sndlen <= conn->mss);
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
         }
+#endif
 
-      conn->nrtx = 0;
 #endif
 
       /* Then handle the rest of the operation just as for the rexmit case */
diff --git a/net/tcp/tcp_sendfile.c b/net/tcp/tcp_sendfile.c
index 87d902f..a1dac31 100644
--- a/net/tcp/tcp_sendfile.c
+++ b/net/tcp/tcp_sendfile.c
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
    */
 
   net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+  conn->sendfile = true;
+#endif
   memset(&state, 0, sizeof(struct sendfile_s));
 
   /* This semaphore is used for signaling and, hence, should not have
@@ -574,6 +577,9 @@ errout_datacb:
 
 errout_locked:
   nxsem_destroy(&state.snd_sem);
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+  conn->sendfile = false;
+#endif
   net_unlock();
 
   /* Return the current file position */
diff --git a/net/tcp/tcp_timer.c b/net/tcp/tcp_timer.c
index a206a09..a8e93f4 100644
--- a/net/tcp/tcp_timer.c
+++ b/net/tcp/tcp_timer.c
@@ -269,7 +269,12 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
 
               else if (
 #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 ||
+#  endif
 #else
                   conn->nrtx >= TCP_MAXRTX ||
 #endif