netconn/sockets: remove fatal error handling, fix asynchronous error handling, ensure data before RST can be received

CHANGELOG

@@ -6,6 +6,10 @@ HISTORY
 
   ++ New features:
 
+  2017-04-11: Simon Goldschmidt
+  * netconn/sockets: remove fatal error handling, fix asynchronous error handling,
+    ensure data before RST can be received
+
   2017-03-02: Simon Goldschmidt
   * netconn: added nonblocking accept/recv to netconn API (task #14396)
 

src/api/api_lib.c

@@ -387,6 +387,7 @@ netconn_listen_with_backlog(struct netconn *conn, u8_t backlog)
 err_t
 netconn_accept(struct netconn *conn, struct netconn **new_conn)
 {
+  err_t err;
 #if LWIP_TCP
   void *accept_ptr;
   struct netconn *newconn;
@@ -398,10 +399,18 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn)
   *new_conn = NULL;
   LWIP_ERROR("netconn_accept: invalid conn",       (conn != NULL),                      return ERR_ARG;);
 
-  if (ERR_IS_FATAL(conn->last_err)) {
-    /* don't recv on fatal errors: this might block the application task
+  /* NOTE: Although the opengroup spec says a pending error shall be returned to
+           send/recv/getsockopt(SO_ERROR) only, we return it for listening
+           connections also, to handle embedded-system errors */
+  err = netconn_err(conn);
+  if (err != ERR_OK) {
+    /* return pending error */
+    return err;
+  }
+  if (conn->flags & NETCONN_FLAG_MBOXCLOSED) {
+    /* don't accept if closed: this might block the application task
        waiting on acceptmbox forever! */
-    return conn->last_err;
+    return ERR_CLSD;
   }
   if (!sys_mbox_valid(&conn->acceptmbox)) {
     return ERR_CLSD;
@@ -432,29 +441,24 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn)
     sys_arch_mbox_fetch(&conn->acceptmbox, &accept_ptr, 0);
 #endif /* LWIP_SO_RCVTIMEO*/
   }
-  newconn = (struct netconn *)accept_ptr;
   /* Register event with callback */
   API_EVENT(conn, NETCONN_EVT_RCVMINUS, 0);
 
-  if (accept_ptr == &netconn_aborted) {
-    /* a connection has been aborted: out of pcbs or out of netconns during accept */
-    /* @todo: set netconn error, but this would be fatal and thus block further accepts */
+  if (lwip_netconn_is_err_msg(accept_ptr, &err)) {
+    /* a connection has been aborted: e.g. out of pcbs or out of netconns during accept */
 #if TCP_LISTEN_BACKLOG
     API_MSG_VAR_FREE(msg);
 #endif /* TCP_LISTEN_BACKLOG */
-    return ERR_ABRT;
+    return err;
   }
-  if (newconn == NULL) {
+  if (accept_ptr == NULL) {
     /* connection has been aborted */
-    /* in this special case, we set the netconn error from application thread, as
-       on a ready-to-accept listening netconn, there should not be anything running
-       in tcpip_thread */
-    NETCONN_SET_SAFE_ERR(conn, ERR_CLSD);
 #if TCP_LISTEN_BACKLOG
     API_MSG_VAR_FREE(msg);
 #endif /* TCP_LISTEN_BACKLOG */
     return ERR_CLSD;
   }
+  newconn = (struct netconn *)accept_ptr;
 #if TCP_LISTEN_BACKLOG
   /* Let the stack know that we have accepted the connection. */
   API_MSG_VAR_REF(msg).conn = newconn;
@@ -494,23 +498,27 @@ netconn_recv_data(struct netconn *conn, void **new_buf, u8_t apiflags)
 {
   void *buf = NULL;
   u16_t len;
+  err_t err;
 
   LWIP_ERROR("netconn_recv: invalid pointer", (new_buf != NULL), return ERR_ARG;);
   *new_buf = NULL;
   LWIP_ERROR("netconn_recv: invalid conn",    (conn != NULL),    return ERR_ARG;);
-  LWIP_ERROR("netconn_recv: invalid recvmbox", sys_mbox_valid(&conn->recvmbox), return ERR_CONN;);
 
-  if (ERR_IS_FATAL(conn->last_err)) {
-    /* don't recv on fatal errors: this might block the application task
-       waiting on recvmbox forever! */
-    /* @todo: this does not allow us to fetch data that has been put into recvmbox
-       before the fatal error occurred - is that a problem? */
-    return conn->last_err;
+  err = netconn_err(conn);
+  if (err != ERR_OK) {
+    /* return pending error */
+    return err;
+  }
+  if (!sys_mbox_valid(&conn->recvmbox)) {
+    return ERR_CLSD;
   }
 
-  if (netconn_is_nonblocking(conn) || (apiflags & NETCONN_DONTBLOCK)) {
+  if (netconn_is_nonblocking(conn) || (apiflags & NETCONN_DONTBLOCK) || (conn->flags & NETCONN_FLAG_MBOXCLOSED)) {
     if (sys_arch_mbox_tryfetch(&conn->recvmbox, &buf) == SYS_ARCH_TIMEOUT) {
-     return ERR_WOULDBLOCK;
+      if (conn->flags & NETCONN_FLAG_MBOXCLOSED) {
+        return ERR_CLSD;
+      }
+      return ERR_WOULDBLOCK;
     }
   } else {
 #if LWIP_SO_RCVTIMEO
@@ -527,10 +535,14 @@ netconn_recv_data(struct netconn *conn, void **new_buf, u8_t apiflags)
   if (NETCONNTYPE_GROUP(conn->type) == NETCONN_TCP)
 #endif /* (LWIP_UDP || LWIP_RAW) */
   {
-    /* If we received a NULL pointer, we are closed */
-    if (buf == NULL) {
-      /* new_buf has been zeroed above alredy */
-      return ERR_OK;
+    /* Check if this is an error message or a pbuf */
+    if (lwip_netconn_is_err_msg(buf, &err)) {
+      /* new_buf has been zeroed above already */
+      if (err == ERR_CLSD) {
+        /* connection closed translates to ERR_OK with *new_buf == NULL */
+        return ERR_OK;
+      }
+      return err;
     }
     len = ((struct pbuf *)buf)->tot_len;
   }
@@ -630,7 +642,11 @@ netconn_recv_data_tcp(struct netconn *conn, struct pbuf **new_buf, u8_t apiflags
     API_EVENT(conn, NETCONN_EVT_RCVMINUS, 0);
     if (conn->pcb.ip == NULL) {
       /* race condition: RST during recv */
-      return conn->last_err == ERR_OK ? ERR_RST : conn->last_err;
+      err = netconn_err(conn);
+      if (err != ERR_OK) {
+        return err;
+      }
+      return ERR_RST;
     }
     /* RX side is closed, so deallocate the recvmbox */
     netconn_close_shutdown(conn, NETCONN_SHUT_RD);
@@ -994,6 +1010,28 @@ netconn_close(struct netconn *conn)
   return netconn_close_shutdown(conn, NETCONN_SHUT_RDWR);
 }
 
+/**
+ * @ingroup netconn_common
+ * Get and reset pending error on a netconn
+ *
+ * @param conn the netconn to get the error from
+ * @return and pending error or ERR_OK if no error was pending
+ */
+err_t
+netconn_err(struct netconn *conn)
+{
+  err_t err;
+  SYS_ARCH_DECL_PROTECT(lev);
+  if (conn == NULL) {
+    return ERR_OK;
+  }
+  SYS_ARCH_PROTECT(lev);
+  err = conn->pending_err;
+  conn->pending_err = ERR_OK;
+  SYS_ARCH_UNPROTECT(lev);
+  return err;
+}
+
 /**
  * @ingroup netconn_tcp
  * Shut down one or both sides of a TCP netconn (doesn't delete it).

src/api/sockets.c

@@ -210,8 +210,6 @@ union lwip_sock_lastdata {
 struct lwip_sock {
   /** sockets currently are built on netconns, each socket has one netconn */
   struct netconn *conn;
-  /** last error that occurred on this socket */
-  int err;
   /** data that was left from the previous read */
   union lwip_sock_lastdata lastdata;
 #if LWIP_SOCKET_SELECT
@@ -325,7 +323,6 @@ static struct lwip_select_cb *select_cb_list;
 
 #define sock_set_errno(sk, e) do { \
   const int sockerr = (e); \
-  sk->err = sockerr; \
   set_errno(sockerr); \
 } while (0)
 
@@ -502,7 +499,6 @@ alloc_socket(struct netconn *newconn, int accepted)
       sockets[i].sendevent  = (NETCONNTYPE_GROUP(newconn->type) == NETCONN_TCP ? (accepted != 0) : 1);
       sockets[i].errevent   = 0;
 #endif /* LWIP_SOCKET_SELECT */
-      sockets[i].err        = 0;
       return i + LWIP_SOCKET_OFFSET;
     }
     SYS_ARCH_UNPROTECT(lev);
@@ -534,9 +530,8 @@ free_socket(struct lwip_sock *sock, int is_tcp)
   }
 #endif
 
-  lastdata         = sock->lastdata;
+  lastdata = sock->lastdata;
   sock->lastdata.pbuf = NULL;
-  sock->err        = 0;
   sock->conn = NULL;
   SYS_ARCH_UNPROTECT(lev);
   /* don't use 'sock' after this line, as another task might have allocated it */
@@ -2422,12 +2417,7 @@ lwip_getsockopt_impl(int s, int level, int optname, void *optval, socklen_t *opt
 
     case SO_ERROR:
       LWIP_SOCKOPT_CHECK_OPTLEN(sock, *optlen, int);
-      /* only overwrite ERR_OK or temporary errors */
-      if (((sock->err == 0) || (sock->err == EINPROGRESS)) && (sock->conn != NULL)) {
-        sock_set_errno(sock, err_to_errno(sock->conn->last_err));
-      }
-      *(int *)optval = sock->err;
-      sock->err = 0;
+      *(int *)optval = err_to_errno(netconn_err(sock->conn));
       LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_getsockopt(%d, SOL_SOCKET, SO_ERROR) = %d\n",
                   s, *(int *)optval));
       break;
@@ -3243,6 +3233,8 @@ lwip_fcntl(int s, int cmd, int val)
 
     break;
   case F_SETFL:
+    /* Bits corresponding to the file access mode and the file creation flags [..] that are set in arg shall be ignored */
+    val &= ~(O_RDONLY|O_WRONLY|O_RDWR);
     if ((val & ~O_NONBLOCK) == 0) {
       /* only O_NONBLOCK, all other bits are zero */
       netconn_set_nonblocking(sock->conn, val & O_NONBLOCK);

src/include/lwip/api.h

@@ -66,6 +66,8 @@ extern "C" {
 #define NETCONN_NOAUTORCVD  0x08 /* prevent netconn_recv_data_tcp() from updating the tcp window - must be done manually via netconn_tcp_recvd() */
 
 /* Flags for struct netconn.flags (u8_t) */
+/** This netconn had an error, don't block on recvmbox/acceptmbox any more */
+#define NETCONN_FLAG_MBOXCLOSED               0x01
 /** Should this netconn avoid blocking? */
 #define NETCONN_FLAG_NON_BLOCKING             0x02
 /** Was the last connect action a non-blocking one? */
@@ -215,8 +217,8 @@ struct netconn {
     struct udp_pcb *udp;
     struct raw_pcb *raw;
   } pcb;
-  /** the last error this netconn had */
-  err_t last_err;
+  /** the last asynchronous unreported error this netconn had */
+  err_t pending_err;
 #if !LWIP_NETCONN_SEM_PER_THREAD
   /** sem that is used to synchronously execute functions in the core context */
   sys_sem_t op_completed;
@@ -278,16 +280,6 @@ struct netvector {
                            (*c->callback)(c, e, l); \
                          }
 
-/** Set conn->last_err to err but don't overwrite fatal errors */
-#define NETCONN_SET_SAFE_ERR(conn, err) do { if ((conn) != NULL) { \
-  SYS_ARCH_DECL_PROTECT(netconn_set_safe_err_lev); \
-  SYS_ARCH_PROTECT(netconn_set_safe_err_lev); \
-  if (!ERR_IS_FATAL((conn)->last_err)) { \
-    (conn)->last_err = err; \
-  } \
-  SYS_ARCH_UNPROTECT(netconn_set_safe_err_lev); \
-}} while(0);
-
 /* Network connection functions: */
 
 /** @ingroup netconn_common
@@ -348,7 +340,7 @@ err_t   netconn_gethostbyname(const char *name, ip_addr_t *addr);
 #endif /* LWIP_IPV4 && LWIP_IPV6 */
 #endif /* LWIP_DNS */
 
-#define netconn_err(conn)               ((conn)->last_err)
+err_t   netconn_err(struct netconn *conn);
 #define netconn_recv_bufsize(conn)      ((conn)->recv_bufsize)
 
 /** Set the blocking status of netconn calls (@todo: write/send is missing) */

src/include/lwip/err.h

@@ -96,8 +96,6 @@ typedef enum {
   ERR_ARG        = -16
 } err_enum_t;
 
-#define ERR_IS_FATAL(e) ((e) <= ERR_ABRT)
-
 /**
  * @}
  */

src/include/lwip/priv/api_msg.h

@@ -185,10 +185,7 @@ struct dns_api_msg {
 };
 #endif /* LWIP_DNS */
 
-#if LWIP_TCP
-extern u8_t netconn_aborted;
-#endif /* LWIP_TCP */
-
+int lwip_netconn_is_err_msg(void *msg, err_t *err);
 void lwip_netconn_do_newconn         (void *m);
 void lwip_netconn_do_delconn         (void *m);
 void lwip_netconn_do_bind            (void *m);