Fixing spurious recv timeout errors and fixing compile warnings

When using the web interface, the web browser will sometimes hold on to
a connection on a tcp socket even though it is not currently
transferring any data. When trying to read from such a connection the
recv system call would fail with either EAGAIN or EWOULDBLOCK indicating
that the attempt to read from the socket had timed out. This patch
changes the attempted reads from the socket to use polling so that a
timeout should only occur in the event of an actual error.

4 files changed, 80 insertions, 112 deletions

diff --git a/src/ippusbxd.c b/src/ippusbxd.c
index 2416575..3bd531d 100644
--- a/src/ippusbxd.c
+++ b/src/ippusbxd.c
@@ -81,7 +81,7 @@ static int is_socket_open(const struct service_thread_param *param);
 /* Global variables */
 static pthread_mutex_t thread_register_mutex;
 static struct service_thread_param **service_threads = NULL;
-static int num_service_threads = 0;
+static uint32_t num_service_threads = 0;
 
 static void sigterm_handler(int sig)
 {
@@ -90,10 +90,11 @@ static void sigterm_handler(int sig)
   NOTE("Caught signal %d, shutting down ...", sig);
 }
 
-static void list_service_threads(int num_service_threads,
-				 struct service_thread_param **service_threads)
+static void list_service_threads(
+    uint32_t num_service_threads,
+    struct service_thread_param **service_threads)
 {
-  int i;
+  uint32_t i;
   char *p;
   char buf[10240];
 
@@ -115,7 +116,8 @@ static void list_service_threads(int num_service_threads,
 }
 
 static int register_service_thread(
-    int *num_service_threads, struct service_thread_param ***service_threads,
+    uint32_t *num_service_threads,
+    struct service_thread_param ***service_threads,
     struct service_thread_param *new_thread)
 {
   NOTE("Registering thread #%u", new_thread->thread_num);
@@ -133,10 +135,10 @@ static int register_service_thread(
 }
 
 static int unregister_service_thread(
-    int *num_service_threads, struct service_thread_param ***service_threads,
-    uint32_t thread_num)
+    uint32_t *num_service_threads,
+    struct service_thread_param ***service_threads, uint32_t thread_num)
 {
-  int i;
+  uint32_t i;
 
   NOTE("Unregistering thread #%u", thread_num);
   /* Search |service_threads| for an element with a matching thread number. */
@@ -339,18 +341,17 @@ static void service_socket_connection(struct service_thread_param *params)
 {
   uint32_t thread_num = params->thread_num;
 
-  struct http_packet_t *pkt = NULL;
-
   while (is_socket_open(params) && !g_options.terminate) {
-    /* Allocate packet for incoming message. */
-    struct http_packet_t *pkt = packet_new();
-    if (pkt == NULL) {
-      ERR("Failed to allocate packet for incoming tcp message");
+    int result = poll_tcp_socket(params->tcp);
+    if (result < 0 || !is_socket_open(params)) {
+      NOTE("Thread #%u: Client closed connection", thread_num);
       return;
+    } else if (result == 0) {
+      continue;
     }
 
-    int status = tcp_packet_get(params->tcp, pkt);
-    if (status) {
+    struct http_packet_t *pkt = tcp_packet_get(params->tcp);
+    if (pkt == NULL) {
       NOTE("Thread #%u: There was an error reading from the socket",
            thread_num);
       return;
@@ -361,13 +362,11 @@ static void service_socket_connection(struct service_thread_param *params)
       return;
     }
 
-    if (pkt->filled_size) {
-      NOTE("Thread #%u: Pkt from tcp (buffer size: %zu)\n===\n%s===", thread_num,
-           pkt->filled_size, hexdump(pkt->buffer, (int)pkt->filled_size));
+    NOTE("Thread #%u: Pkt from tcp (buffer size: %zu)\n===\n%s===", thread_num,
+         pkt->filled_size, hexdump(pkt->buffer, (int)pkt->filled_size));
 
-      /* Send pkt to printer. */
-      usb_conn_packet_send(params->usb_conn, pkt);
-    }
+    /* Send pkt to printer. */
+    usb_conn_packet_send(params->usb_conn, pkt);
 
     packet_free(pkt);
   }
@@ -623,25 +622,7 @@ static void start_daemon()
   if (usb_sock == NULL) goto cleanup_usb;
 
   /* Capture a socket */
-  uint16_t desired_port = g_options.desired_port;
-  g_options.tcp_socket = NULL;
-  g_options.tcp6_socket = NULL;
-  for (;;) {
-    g_options.tcp_socket = tcp_open(desired_port, g_options.interface);
-    g_options.tcp6_socket = tcp6_open(desired_port, g_options.interface);
-    if (g_options.tcp_socket || g_options.tcp6_socket || g_options.only_desired_port)
-      break;
-    /* Search for a free port */
-    desired_port ++;
-    /* We failed with 0 as port number or we reached the max
-       port number */
-    if (desired_port == 1 || desired_port == 0)
-      /* IANA recommendation of 49152 to 65535 for ephemeral
-	 ports
-	 https://en.wikipedia.org/wiki/Ephemeral_port */
-      desired_port = 49152;
-    NOTE("Access to desired port failed, trying alternative port %d", desired_port);
-  }
+  uint16_t desired_port = open_tcp_socket();
   if (g_options.tcp_socket == NULL && g_options.tcp6_socket == NULL)
     goto cleanup_tcp;
 
diff --git a/src/tcp.c b/src/tcp.c
index 46fa78c..92a128c 100644
--- a/src/tcp.c
+++ b/src/tcp.c
@@ -222,36 +222,30 @@ uint16_t tcp_port_number_get(struct tcp_sock_t *sock)
   return 0;
 }
 
-int tcp_packet_get(struct tcp_conn_t *tcp, struct http_packet_t *pkt)
+struct http_packet_t *tcp_packet_get(struct tcp_conn_t *tcp)
 {
+  /* Allocate packet for incoming message. */
+  struct http_packet_t *pkt = packet_new();
+  if (pkt == NULL) {
+    ERR("failed to create packet for incoming tcp message");
+    goto error;
+  }
+
   struct timeval tv;
-  tv.tv_sec = 5;
+  tv.tv_sec = 3;
   tv.tv_usec = 0;
   if (setsockopt(tcp->sd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv))) {
     ERR("TCP: Setting options for tcp connection socket failed");
-    return -1;
+    goto error;
   }
 
   ssize_t gotten_size = recv(tcp->sd, pkt->buffer, pkt->buffer_capacity, 0);
 
   if (gotten_size < 0) {
     int errno_saved = errno;
-    /* In the event that there was a timeout reading from the socket, but there
-     * has recently been data sent from the printer to the socket, then we do
-     * not return an error and simply allow for another attempt at reading from
-     * the socket. */
-    if ((errno_saved == EAGAIN || errno_saved == EWOULDBLOCK) &&
-        get_is_active(tcp)) {
-      /* Mark the the socket as inactive so that if another timeout occurs
-       * without new data being transferred on the socket then the connection
-       * will close. */
-      set_is_active(tcp, 0);
-      return 0;
-    }
-
     ERR("recv failed with err %d:%s", errno_saved, strerror(errno_saved));
     tcp->is_closed = 1;
-    return -1;
+    goto error;
   }
 
   if (gotten_size == 0) {
@@ -259,7 +253,12 @@ int tcp_packet_get(struct tcp_conn_t *tcp, struct http_packet_t *pkt)
   }
 
   pkt->filled_size = gotten_size;
-  return 0;
+  return pkt;
+
+ error:
+  if (pkt != NULL)
+    packet_free(pkt);
+  return NULL;
 }
 
 int tcp_packet_send(struct tcp_conn_t *conn, struct http_packet_t *pkt)
@@ -279,11 +278,12 @@ int tcp_packet_send(struct tcp_conn_t *conn, struct http_packet_t *pkt)
       return -1;
     }
 
-    total += sent;
-    if (sent >= remaining)
+    size_t sent_unsigned = (size_t)sent;
+    total += sent_unsigned;
+    if (sent_unsigned >= remaining)
       remaining = 0;
     else
-      remaining -= sent;
+      remaining -= sent_unsigned;
   }
 
   NOTE("TCP: sent %lu bytes", total);
@@ -365,6 +365,40 @@ void tcp_conn_close(struct tcp_conn_t *conn)
   free(conn);
 }
 
+/* Poll the tcp socket to determine if it is ready to transmit data. */
+int poll_tcp_socket(struct tcp_conn_t *tcp)
+{
+  struct pollfd poll_fd;
+  poll_fd.fd = tcp->sd;
+  poll_fd.events = POLLIN;
+  const int nfds = 1;
+  const int timeout = 5000;  /* 5 seconds. */
+
+  int result = poll(&poll_fd, nfds, timeout);
+  if (result < 0) {
+    ERR("poll failed with error %d:%s", errno, strerror(errno));
+    tcp->is_closed = 1;
+  } else if (result == 0) {
+    /* In the case where the poll timed out, check to see whether or not data
+     * has recently been sent from the printer along the socket. If so, then we
+     * keep the connection alive an reset the is_active flag. Otherwise, close
+     * the connection. */
+    if (get_is_active(tcp)) {
+      set_is_active(tcp, 0);
+    } else {
+      tcp->is_closed = 1;
+    }
+  } else {
+    if (poll_fd.revents != POLLIN) {
+      ERR("poll returned an unexpected event");
+      tcp->is_closed = 1;
+      return -1;
+    }
+  }
+
+  return result;
+}
+
 int get_is_active(struct tcp_conn_t *tcp)
 {
   pthread_mutex_lock(&tcp->mutex);
diff --git a/src/tcp.h b/src/tcp.h
index e79edb4..8231119 100644
--- a/src/tcp.h
+++ b/src/tcp.h
@@ -52,8 +52,10 @@ struct tcp_conn_t *tcp_conn_select(struct tcp_sock_t *sock,
 				   struct tcp_sock_t *sock6);
 void tcp_conn_close(struct tcp_conn_t *);
 
-int tcp_packet_get(struct tcp_conn_t *, struct http_packet_t *);
+struct http_packet_t *tcp_packet_get(struct tcp_conn_t *);
 int tcp_packet_send(struct tcp_conn_t *, struct http_packet_t *);
 
+int poll_tcp_socket(struct tcp_conn_t *tcp);
+
 int get_is_active(struct tcp_conn_t *tcp);
 void set_is_active(struct tcp_conn_t *tcp, int val);
diff --git a/src/usb.c b/src/usb.c
index cadc006..fa8ce59 100644
--- a/src/usb.c
+++ b/src/usb.c
@@ -609,55 +609,6 @@ void usb_register_callback(struct usb_sock_t *usb)
     ERR("Failed to register unplug callback");
 }
 
-static void usb_conn_mark_staled(struct usb_conn_t *conn)
-{
-  if (conn->is_staled)
-    return;
-
-  struct usb_sock_t *usb = conn->parent;
-
-  sem_wait(&usb->num_staled_lock);
-  {
-    usb->num_staled++;
-  }
-  sem_post(&usb->num_staled_lock);
-
-  conn->is_staled = 1;
-}
-
-static void usb_conn_mark_moving(struct usb_conn_t *conn)
-{
-  if (!conn->is_staled)
-    return;
-
-  struct usb_sock_t *usb = conn->parent;
-
-  sem_wait(&usb->num_staled_lock);
-  {
-    usb->num_staled--;
-  }
-  sem_post(&usb->num_staled_lock);
-
-  conn->is_staled = 0;
-}
-
-static int usb_all_conns_staled(struct usb_sock_t *usb)
-{
-  int staled;
-
-  sem_wait(&usb->num_staled_lock);
-  {
-    sem_wait(&usb->pool_manage_lock);
-    {
-      staled = usb->num_staled == usb->num_taken;
-    }
-    sem_post(&usb->pool_manage_lock);
-  }
-  sem_post(&usb->num_staled_lock);
-
-  return staled;
-}
-
 struct usb_conn_t *usb_conn_acquire(struct usb_sock_t *usb)
 {
   int i;