api: comment, simplify and fix api socket read

The function vl_socket_read_ready did contain some comments already,
but as they stated, the logic has to be tricky to cover multiple cases.

Comment:
+ Add function-level comment
+ Add comments to describe some of local variables
+ Add many comments to describe internal state at particular lines.

Simplify:
+ Remov mbp_set as it is never needed.
+ Replace msg_len with msgbuf_len to save "+ sizeof (msgbuf_t)".

Improve:
+ Early exit on EAGAIN.

Fix:
+ "n" now only tracks input_buffer.
  Previously, it was entering the detection of additional messages
  even for unprocessed_input.
+ Set up msg_buffer (including appending to unprocessed_input)
  outside full-message-detection loop now,
  so it cannot be executed multiple times as before.

Type: fix
Ticket: VPP-1785

Change-Id: I256e34b435be06844458744a13ea37a0e86a96f9
Signed-off-by: Vratko Polak <vrpolak@cisco.com>

1 files changed, 80 insertions, 48 deletions

diff --git a/src/vlibmemory/socket_api.c b/src/vlibmemory/socket_api.c
index 868298ccc85..a07d717e1d1 100644
--- a/src/vlibmemory/socket_api.c
+++ b/src/vlibmemory/socket_api.c
@@ -203,43 +203,68 @@ vl_socket_process_api_msg (clib_file_t * uf, vl_api_registration_t * rp,
   socket_main.current_rp = 0;
 }
 
+/*
+ * Read function for API socket.
+ *
+ * Read data from socket, invoke SOCKET_READ_EVENT
+ * for each fully read API message, return 0.
+ * Store incomplete data for next invocation to continue.
+ *
+ * On severe read error, the file is closed.
+ *
+ * As reading is single threaded,
+ * socket_main.input_buffer is used temporarily.
+ * Even its length is modified, but always restored before return.
+ *
+ * Incomplete data is copied into a vector,
+ * pointer saved in registration's unprocessed_input.
+ */
 clib_error_t *
 vl_socket_read_ready (clib_file_t * uf)
 {
   clib_file_main_t *fm = &file_main;
   vlib_main_t *vm = vlib_get_main ();
   vl_api_registration_t *rp;
+  /* n is the size of data read to input_buffer */
   int n;
+  /* msg_buffer vector can point to input_buffer or unprocessed_input */
   i8 *msg_buffer = 0;
+  /* data_for_process is a vector containing one full message, incl msgbuf_t */
   u8 *data_for_process;
-  u32 msg_len;
+  /* msgbuf_len is the size of one message, including sizeof (msgbuf_t) */
+  u32 msgbuf_len;
   u32 save_input_buffer_length = vec_len (socket_main.input_buffer);
   vl_socket_args_for_process_t *a;
-  msgbuf_t *mbp;
-  int mbp_set = 0;
 
   rp = pool_elt_at_index (socket_main.registration_pool, uf->private_data);
 
+  /* Ignore unprocessed_input for now, n describes input_buffer for now. */
   n = read (uf->file_descriptor, socket_main.input_buffer,
 	    vec_len (socket_main.input_buffer));
 
-  if (n <= 0 && errno != EAGAIN)
+  if (n <= 0)
     {
-      clib_file_del (fm, uf);
-
-      if (!pool_is_free (socket_main.registration_pool, rp))
-	{
-	  u32 index = rp - socket_main.registration_pool;
-	  vl_socket_free_registration_index (index);
-	}
-      else
+      if (errno != EAGAIN)
 	{
-	  clib_warning ("client index %d already free?",
-			rp->vl_api_registration_pool_index);
+	  /* Severe error, close the file. */
+	  clib_file_del (fm, uf);
+
+	  if (!pool_is_free (socket_main.registration_pool, rp))
+	    {
+	      u32 index = rp - socket_main.registration_pool;
+	      vl_socket_free_registration_index (index);
+	    }
+	  else
+	    {
+	      clib_warning ("client index %d already free?",
+			    rp->vl_api_registration_pool_index);
+	    }
 	}
+      /* EAGAIN means we do not close the file, but no data to process anyway. */
       return 0;
     }
 
+  /* Fake smaller length teporarily, so input_buffer can be used as msg_buffer. */
   _vec_len (socket_main.input_buffer) = n;
 
   /*
@@ -248,37 +273,38 @@ vl_socket_read_ready (clib_file_t * uf)
    * boundaries. In the case of a long message (>4K bytes)
    * we have to do (at least) 2 reads, etc.
    */
+  /* Determine msg_buffer. */
+  if (vec_len (rp->unprocessed_input))
+    {
+      vec_append (rp->unprocessed_input, socket_main.input_buffer);
+      msg_buffer = rp->unprocessed_input;
+    }
+  else
+    {
+      msg_buffer = socket_main.input_buffer;
+    }
+  /* Loop to process any full messages. */
+  ASSERT (vec_len (msg_buffer) > 0);
   do
     {
-      if (vec_len (rp->unprocessed_input))
-	{
-	  vec_append (rp->unprocessed_input, socket_main.input_buffer);
-	  msg_buffer = rp->unprocessed_input;
-	}
-      else
-	{
-	  msg_buffer = socket_main.input_buffer;
-	  mbp_set = 0;
-	}
-
-      if (mbp_set == 0)
-	{
-	  /* Any chance that we have a complete message? */
-	  if (vec_len (msg_buffer) <= sizeof (msgbuf_t))
-	    goto save_and_split;
-
-	  mbp = (msgbuf_t *) msg_buffer;
-	  msg_len = ntohl (mbp->data_len);
-	  mbp_set = 1;
-	}
-
-      /* We don't have the entire message yet. */
-      if (mbp_set == 0
-	  || (msg_len + sizeof (msgbuf_t)) > vec_len (msg_buffer))
+      /* Here, we are not sure how big a chunk of message we have left. */
+      /* Do we at least know how big the full message will be? */
+      if (vec_len (msg_buffer) <= sizeof (msgbuf_t))
+	/* No, so fragment is not a full message. */
+	goto save_and_split;
+
+      /* Now we know how big the full message will be. */
+      msgbuf_len =
+	ntohl (((msgbuf_t *) msg_buffer)->data_len) + sizeof (msgbuf_t);
+
+      /* But do we have a full message? */
+      if (msgbuf_len > vec_len (msg_buffer))
 	{
 	save_and_split:
-	  /* if we were using the input buffer save the fragment */
+	  /* We don't have the entire message yet. */
+	  /* If msg_buffer is unprocessed_input, nothing needs to be done. */
 	  if (msg_buffer == socket_main.input_buffer)
+	    /* But if we were using the input buffer, save the fragment. */
 	    {
 	      ASSERT (vec_len (rp->unprocessed_input) == 0);
 	      vec_validate (rp->unprocessed_input, vec_len (msg_buffer) - 1);
@@ -286,12 +312,19 @@ vl_socket_read_ready (clib_file_t * uf)
 				vec_len (msg_buffer));
 	      _vec_len (rp->unprocessed_input) = vec_len (msg_buffer);
 	    }
+	  /* No more full messages, restore original input_buffer length. */
 	  _vec_len (socket_main.input_buffer) = save_input_buffer_length;
 	  return 0;
 	}
 
+      /*
+       * We have at least one full message.
+       * But msg_buffer can contain more data, so copy one message data
+       * so we can overwrite its length to what single message has.
+       */
       data_for_process = (u8 *) vec_dup (msg_buffer);
-      _vec_len (data_for_process) = (msg_len + sizeof (msgbuf_t));
+      _vec_len (data_for_process) = msgbuf_len;
+      /* Everything is ready to signal the SOCKET_READ_EVENT. */
       pool_get (socket_main.process_args, a);
       a->clib_file = uf;
       a->regp = rp;
@@ -300,18 +333,17 @@ vl_socket_read_ready (clib_file_t * uf)
       vlib_process_signal_event (vm, vl_api_clnt_node.index,
 				 SOCKET_READ_EVENT,
 				 a - socket_main.process_args);
-      if (n > (msg_len + sizeof (*mbp)))
-	vec_delete (msg_buffer, msg_len + sizeof (*mbp), 0);
+      if (vec_len (msg_buffer) > msgbuf_len)
+	/* There are some fragments left. Shrink the msg_buffer to simplify logic. */
+	vec_delete (msg_buffer, msgbuf_len, 0);
       else
+	/* We are done with msg_buffer. */
 	_vec_len (msg_buffer) = 0;
-      n -= msg_len + sizeof (msgbuf_t);
-      msg_len = 0;
-      mbp_set = 0;
     }
-  while (n > 0);
+  while (vec_len (msg_buffer) > 0);
 
+  /* Restore input_buffer, it could have been msg_buffer. */
   _vec_len (socket_main.input_buffer) = save_input_buffer_length;
-
   return 0;
 }