From f8b785f10e2d5a2c0abb06413611d3667c7617dd Mon Sep 17 00:00:00 2001
From: patacongo <patacongo@42af7a65-404d-4744-a932-0658087f49c3>
Date: Sat, 18 Feb 2012 18:13:30 +0000
Subject: [PATCH] Correct and error in recv() and recvfrom() return value

git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@4402 42af7a65-404d-4744-a932-0658087f49c3
---
 ChangeLog                         |  2 +
 Documentation/NuttxUserGuide.html |  8 ++--
 TODO                              | 16 +-------
 net/accept.c                      |  5 ++-
 net/connect.c                     | 33 +++++++++++++---
 net/net_internal.h                | 30 +++++++++-----
 net/recvfrom.c                    | 66 +++++++++++++++++--------------
 net/sendto.c                      | 13 +-----
 8 files changed, 97 insertions(+), 76 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 62e11f6bb64..138ab78b233 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2467,3 +2467,5 @@
 	  a lower-half quadrature encoder driver for the STM32.  On initial check-in,
 	  this is little more than a "skeleton" file.
 	* Various files:  CAN ISO-11783 support contributed by Gary Teravskis.
+	* net/recv.c and net/recvfrom.c: Correct a bug in return value:  The the peer
+	  gracefully closes the connections, needs to return zero and not ENOTCONN
\ No newline at end of file
diff --git a/Documentation/NuttxUserGuide.html b/Documentation/NuttxUserGuide.html
index 78f79b38293..614570aa926 100644
--- a/Documentation/NuttxUserGuide.html
+++ b/Documentation/NuttxUserGuide.html
@@ -13,7 +13,7 @@
       <h1><big><font color="#3c34ec"><i>NuttX Operating System<p>User's Manual</i></font></big></h1>
       <p><small>by</small></p>
       <p>Gregory Nutt<p>
-      <p>Last Updated: December 5, 2011</p>
+      <p>Last Updated: February 18, 2011</p>
     </td>
   </tr>
 </table>
@@ -7391,8 +7391,7 @@ Those socket APIs are discussed in the following paragraphs.</p>
 </ul>
 <p>
   <b>Returned Values:</b>
-  see <a href="#recvfrom"><code>recvfrom()</code></a>.
-  Zero on success.
+  See <a href="#recvfrom"><code>recvfrom()</code></a>.
 </p>
 
 <h3><a name="recvfrom">2.12.9 <code>recvfrom</code></a></h3>
@@ -7429,7 +7428,8 @@ Those socket APIs are discussed in the following paragraphs.</p>
 <p>
   <b>Returned Values:</b>
   On success, returns the number of characters sent.
-  On error, -1 is returned, and <a href="#ErrnoAccess"><code>errno</code></a> is set appropriately:
+  If no data is available to be received and the peer has performed an orderly shutdown, recv() will return 0.
+  Othwerwise, on errors, -1 is returned, and <a href="#ErrnoAccess"><code>errno</code></a> is set appropriately:
 </p>
 <ul>
     <li><code>EAGAIN</code>.
diff --git a/TODO b/TODO
index 27701c2d0de..44a607ec4bd 100644
--- a/TODO
+++ b/TODO
@@ -13,7 +13,7 @@ nuttx/
   (1)  pthreads (sched/)
   (2)  C++ Support
   (5)  Binary loaders (binfmt/)
- (18)  Network (net/, drivers/net)
+ (17)  Network (net/, drivers/net)
   (2)  USB (drivers/usbdev, drivers/usbhost)
   (8)  Libraries (lib/)
  (10)  File system/Generic drivers (fs/, drivers/)
@@ -437,20 +437,8 @@ o Network (net/, drivers/net)
   Status:      Open
   Priority:    Low unless you need it.
 
-  Title:       RECV/RECVFROM RETURN VALUE
-  Description: If the peer performs an orderly shutdown, then recvfrom currently returns
-               an error with errno set to ENOTCONN.  This is wrong.  There is a fine
-               distinction.  The ENOTCONN errno is intended for the case where the socket
-               was never connected.  In the case were the socket was connected then the
-               peer performs an order shutdown:  "...If no messages are available to be
-               received and the peer has performed an orderly shutdown, recv() shall
-               return 0. ..."
-  Status:      Open and there is a kludge in apps/netutils/ftpd/ftpdc.c work around this
-               bad return value.
-  Priority:    Medium
-
 o USB (drivers/usbdev, drivers/usbhost)
-  ^^^^^^^^^^^^^^^^^^^^
+  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
   Title:       USB STORAGE DRIVER DELAYS
   Description: There is a workaround for a bug in drivers/usbdev/usbdev_storage.c.
diff --git a/net/accept.c b/net/accept.c
index 27e5d2b0a88..b45eb6cd7d6 100644
--- a/net/accept.c
+++ b/net/accept.c
@@ -1,8 +1,8 @@
 /****************************************************************************
  * net/accept.c
  *
- *   Copyright (C) 2007-2011 Gregory Nutt. All rights reserved.
- *   Author: Gregory Nutt <spudmonkey@racsa.co.cr>
+ *   Copyright (C) 2007-2012 Gregory Nutt. All rights reserved.
+ *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -441,6 +441,7 @@ int accept(int sockfd, struct sockaddr *addr, socklen_t *addrlen)
   pnewsock->s_type   = SOCK_STREAM;
   pnewsock->s_conn   = state.acpt_newconn;
   pnewsock->s_flags |= _SF_CONNECTED;
+  pnewsock->s_flags &= ~_SF_CLOSED;
   return newfd;
 
 errout_with_lock:
diff --git a/net/connect.c b/net/connect.c
index f6006e3e6e3..d07bf92fcfd 100644
--- a/net/connect.c
+++ b/net/connect.c
@@ -116,15 +116,37 @@ static void connection_event(struct uip_conn *conn, uint16_t flags)
     {
       nllvdbg("flags: %04x s_flags: %02x\n", flags, psock->s_flags);
 
-      /* UIP_CLOSE: The remote host has closed the connection
-       * UIP_ABORT: The remote host has aborted the connection
-       * UIP_TIMEDOUT: Connection aborted due to too many retransmissions.
+      /* These loss-of-connection events may be reported:
+       *
+       *   UIP_CLOSE: The remote host has closed the connection
+       *   UIP_ABORT: The remote host has aborted the connection
+       *   UIP_TIMEDOUT: Connection aborted due to too many retransmissions.
+       *
+       * And we need to set these two socket status bits appropriately:
+       *
+       *  _SF_CONNECTED==1 && _SF_CLOSED==0 - the socket is connected
+       *  _SF_CONNECTED==0 && _SF_CLOSED==1 - the socket was gracefully disconnected
+       *  _SF_CONNECTED==0 && _SF_CLOSED==0 - the socket was rudely disconnected
        */
-      if ((flags & (UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT)) != 0)
+
+      if ((flags & UIP_CLOSE) != 0)
         {
-          /* Indicate that the socket is no longer connected */
+          /* The peer gracefully closed the connection.  Marking the
+           * connection as disconnected will suppress some subsequent
+           * ENOTCONN errors from receive.  A graceful disconnection is
+           * not handle as an error but as an "end-of-file"
+           */
 
           psock->s_flags &= ~_SF_CONNECTED;
+          psock->s_flags |= _SF_CLOSED;
+        }
+      else if ((flags & (UIP_ABORT|UIP_TIMEDOUT)) != 0)
+        {
+          /* The loss of connection was less than graceful.  This will (eventually)
+           * be reported as an ENOTCONN error.
+           */
+
+          psock->s_flags &= ~(_SF_CONNECTED |_SF_CLOSED);
         }
 
       /* UIP_CONNECTED: The socket is successfully connected */
@@ -134,6 +156,7 @@ static void connection_event(struct uip_conn *conn, uint16_t flags)
           /* Indicate that the socket is now connected */
 
           psock->s_flags |= _SF_CONNECTED;
+          psock->s_flags &= ~_SF_CLOSED;
         }
     }
 }
diff --git a/net/net_internal.h b/net/net_internal.h
index 190d5781504..e130b42e884 100644
--- a/net/net_internal.h
+++ b/net/net_internal.h
@@ -57,16 +57,25 @@
 /* Definitions of 8-bit socket flags */
 
                                   /* Bits 0-2: Socket state */
-#define _SF_IDLE            0x00  /* There is no socket activity */
-#define _SF_ACCEPT          0x01  /* Socket is waiting to accept a connection */
-#define _SF_RECV            0x02  /* Waiting for recv action to complete */
-#define _SF_SEND            0x03  /* Waiting for send action to complete */
-#define _SF_MASK            0x03  /* Mask to isolate the above actions */
-                                  /* Bit 3: unused */
-#define _SF_NONBLOCK        0x10  /* Bit 4: Don't block if no data (TCP/READ only) */
-#define _SF_LISTENING       0x20  /* Bit 5: SOCK_STREAM is listening */
-#define _SF_BOUND           0x40  /* Bit 6: SOCK_STREAM is bound to an address */
-#define _SF_CONNECTED       0x80  /* Bit 7: SOCK_STREAM is connected */
+#define _SF_IDLE            0x00  /* - There is no socket activity */
+#define _SF_ACCEPT          0x01  /* - Socket is waiting to accept a connection */
+#define _SF_RECV            0x02  /* - Waiting for recv action to complete */
+#define _SF_SEND            0x03  /* - Waiting for send action to complete */
+#define _SF_MASK            0x03  /* - Mask to isolate the above actions */
+
+#define _SF_NONBLOCK        0x08  /* Bit 3: Don't block if no data (TCP/READ only) */
+#define _SF_LISTENING       0x10  /* Bit 4: SOCK_STREAM is listening */
+#define _SF_BOUND           0x20  /* Bit 5: SOCK_STREAM is bound to an address */
+                                  /* Bits 6-7: Connection state */
+#define _SF_CONNECTED       0x40  /* Bit 6: SOCK_STREAM is connected */
+#define _SF_CLOSED          0x80  /* Bit 7: SOCK_STREAM was gracefully disconnected */
+
+/* Connection state encoding:
+ *
+ *  _SF_CONNECTED==1 && _SF_CLOSED==0 - the socket is connected
+ *  _SF_CONNECTED==0 && _SF_CLOSED==1 - the socket was gracefully disconnected
+ *  _SF_CONNECTED==0 && _SF_CLOSED==0 - the socket was rudely disconnected
+ */
 
 /* Macro to manage the socket state and flags */
 
@@ -78,6 +87,7 @@
 #define _SS_ISLISTENING(s)  (((s) & _SF_LISTENING) != 0)
 #define _SS_ISBOUND(s)      (((s) & _SF_CONNECTED) != 0)
 #define _SS_ISCONNECTED(s)  (((s) & _SF_CONNECTED) != 0)
+#define _SS_ISCLOSED(s)     (((s) & _SF_CLOSED) != 0)
 
 /* This macro converts a socket option value into a bit setting */
 
diff --git a/net/recvfrom.c b/net/recvfrom.c
index f1f08d66d27..aabfcedfd3e 100644
--- a/net/recvfrom.c
+++ b/net/recvfrom.c
@@ -419,7 +419,12 @@ static uint16_t recvfrom_tcpinterrupt(struct uip_driver_s *dev, void *conn,
 #endif
         }
 
-      /* Check for a loss of connection */
+      /* Check for a loss of connection.
+       *
+       * UIP_CLOSE: The remote host has closed the connection
+       * UIP_ABORT: The remote host has aborted the connection
+       * UIP_TIMEDOUT: Connection aborted due to too many retransmissions.
+       */
 
       else if ((flags & (UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT)) != 0)
         {
@@ -431,9 +436,18 @@ static uint16_t recvfrom_tcpinterrupt(struct uip_driver_s *dev, void *conn,
           pstate->rf_cb->priv    = NULL;
           pstate->rf_cb->event   = NULL;
 
-          /* Report not connected */
+          /* If the peer gracefully closed the connection, then return zero
+           * (end-of-file).  Otherwise, report a not-connected error
+           */
 
-          pstate->rf_result = -ENOTCONN;
+          if ((flags & UIP_CLOSE) != 0)
+            {
+              pstate->rf_result = 0;
+            }
+          else
+            {
+              pstate->rf_result = -ENOTCONN;
+            }
 
           /* Wake up the waiting thread */
 
@@ -585,27 +599,6 @@ static uint16_t recvfrom_udpinterrupt(struct uip_driver_s *dev, void *pvconn,
           sem_post(&pstate->rf_sem);
         }
 
-      /* Check for a loss of connection */
-
-      else if ((flags & (UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT)) != 0)
-        {
-          nllvdbg("error\n");
-
-          /* Stop further callbacks */
-
-          pstate->rf_cb->flags   = 0;
-          pstate->rf_cb->priv    = NULL;
-          pstate->rf_cb->event   = NULL;
-
-          /* Report not connected */
-
-          pstate->rf_result = -ENOTCONN;
-
-          /* Wake up the waiting thread */
-
-          sem_post(&pstate->rf_sem);
-        }
-
       /* No data has been received -- this is some other event... probably a
        * poll -- check for a timeout.
        */
@@ -719,7 +712,9 @@ static ssize_t recvfrom_result(int result, struct recvfrom_s *pstate)
 
   if (pstate->rf_result < 0)
     {
-      /* Return EGAIN on a timeout or ENOTCONN on loss of connection */
+      /* This might return EGAIN on a timeout or ENOTCONN on loss of
+       * connection (TCP only)
+       */
 
       return pstate->rf_result;
     }
@@ -796,7 +791,7 @@ static ssize_t udp_recvfrom(FAR struct socket *psock, FAR void *buf, size_t len,
     {
       /* Set up the callback in the connection */
 
-      state.rf_cb->flags   = UIP_NEWDATA|UIP_POLL|UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT;
+      state.rf_cb->flags   = UIP_NEWDATA|UIP_POLL;
       state.rf_cb->priv    = (void*)&state;
       state.rf_cb->event   = recvfrom_udpinterrupt;
 
@@ -900,11 +895,20 @@ static ssize_t tcp_recvfrom(FAR struct socket *psock, FAR void *buf, size_t len,
   if (!_SS_ISCONNECTED(psock->s_flags))
     {
       /* Was any data transferred from the readahead buffer after we were
-       * disconnected?
+       * disconnected?  If so, then return the number of bytes received.  We
+       * will wait to return end disconnection indications the next time that
+       * recvfrom() is called.
+       *
+       * If no data was received (i.e.,  ret == 0  -- it will not be negative)
+       * and the connection was gracefully closed by the remote peer, then return
+       * success.  If rf_recvlen is zero, the caller of recvfrom() will get an
+       * end-of-file indication.
        */
 
 #if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0
-      if (ret <= 0)
+      if (ret <= 0 && !_SS_ISCLOSED(psock->s_flags))
+#else
+      if (!_SS_ISCLOSED(psock->s_flags))
 #endif
         {
           /* Nothing was previously received from the readahead buffers.
@@ -1008,8 +1012,10 @@ static ssize_t tcp_recvfrom(FAR struct socket *psock, FAR void *buf, size_t len,
  *   fromlen  The length of the address structure
  *
  * Returned Value:
- *   On success, returns the number of characters sent.  On  error,
- *   -1 is returned, and errno is set appropriately:
+ *   On success, returns the number of characters sent.  If no data is
+ *   available to be received and the peer has performed an orderly shutdown,
+ *   recv() will return 0.  Othwerwise, on errors, -1 is returned, and errno
+ *   is set appropriately:
  *
  *   EAGAIN
  *     The socket is marked non-blocking and the receive operation would block,
diff --git a/net/sendto.c b/net/sendto.c
index 4e1f9015f23..cd3026eacf6 100644
--- a/net/sendto.c
+++ b/net/sendto.c
@@ -103,22 +103,13 @@ static uint16_t sendto_interrupt(struct uip_driver_s *dev, void *conn,
   nllvdbg("flags: %04x\n", flags);
   if (pstate)
     {
-      /* Check if the connection was rejected */
-
-      if ((flags & (UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT)) != 0)
-        {
-          /* Yes.. then terminate with an error */
-
-          pstate->st_sndlen = -ENOTCONN;
-        }
-
       /* Check if the outgoing packet is available (it may have been claimed
        * by a sendto interrupt serving a different thread -OR- if the output
        * buffer currently contains unprocessed incoming data.  In these cases
        * we will just have to wait for the next polling cycle.
        */
 
-      else if (dev->d_sndlen > 0 || (flags & UIP_NEWDATA) != 0)
+      if (dev->d_sndlen > 0 || (flags & UIP_NEWDATA) != 0)
         {
            /* Another thread has beat us sending data or the buffer is busy,
             * wait for the next polling cycle
@@ -314,7 +305,7 @@ ssize_t psock_sendto(FAR struct socket *psock, FAR const void *buf,
   state.st_cb = uip_udpcallbackalloc(conn);
   if (state.st_cb)
     {
-      state.st_cb->flags   = UIP_POLL|UIP_CLOSE|UIP_ABORT|UIP_TIMEDOUT;
+      state.st_cb->flags   = UIP_POLL;
       state.st_cb->priv    = (void*)&state;
       state.st_cb->event   = sendto_interrupt;