Fix the HandleReceivedExplict[Tcp|Udp]Data() return type inconsistency

The mentioned functions sometimes returned only values of EipStatus type
but sometimes the length of the response message to be send. But the
size of the response is in any case already transferred to the caller by
the outgoing_message.used_message_length structure member.

Now both functions return only values of EipStatus type and the callers
evaluate only this kind of status. To make the functions return EipStatus
consistently some lower level functions also had to be changed to return
EipStatus notably NotifyConnectedCommonPacketFormat() and
NotifyCommonPacketFormat().

Some changes are marked with TODO and therefore have to be discussed before
the final commit.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>

source/src/enet_encap/cpf.c

@@ -23,10 +23,13 @@ const size_t sequenced_address_item_length = 8;
 
 CipCommonPacketFormatData g_common_packet_format_data_item; /**< CPF global data items */
 
-int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
-                             const struct sockaddr *const originator_address,
-                             ENIPMessage *const outgoing_message) {
-  int return_value = kEipStatusError;
+EipStatus NotifyCommonPacketFormat
+(
+  const EncapsulationData *const received_data,
+  const struct sockaddr *const originator_address,
+  ENIPMessage *const outgoing_message)
+{
+  EipStatus return_value = kEipStatusError;
 
   if (kEipStatusError == ( return_value = CreateCommonPacketFormatStructure(
                              received_data->
@@ -35,7 +38,7 @@ int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
                              &g_common_packet_format_data_item) ) ) {
     OPENER_TRACE_ERR("notifyCPF: error from createCPFstructure\n");
   } else {
-    return_value = kEipStatusOk; /* In cases of errors we normally need to send an error response */
+    return_value = kEipStatusOkSend;  /* In cases of errors we normally need to send an error response */
     if (g_common_packet_format_data_item.address_item.type_id
         == kCipItemIdNullAddress) /* check if NullAddressItem received, otherwise it is no unconnected message and should not be here*/
     {     /* found null address item*/
@@ -48,7 +51,9 @@ int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
           received_data->session_handle);
         if (return_value != kEipStatusError) {
           SkipEncapsulationHeader(outgoing_message);
-          return_value = AssembleLinearMessage(
+          /* TODO: Here we lose a possible kEipStatusError from AssembleLinearMessage().
+           *  Its not clear how to transport this error information to the requester. */
+          int response_len = AssembleLinearMessage(
             &g_message_router_response, &g_common_packet_format_data_item,
             outgoing_message);
 
@@ -56,44 +61,44 @@ int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
           outgoing_message->current_message_position =
             outgoing_message->message_buffer;
           GenerateEncapsulationHeader(received_data,
-                                      return_value,
+                                      response_len,
                                       received_data->session_handle,
                                       kEncapsulationProtocolSuccess,
                                       outgoing_message);
           outgoing_message->current_message_position = buffer;
-          return_value = outgoing_message->used_message_length;
+          return_value = kEipStatusOkSend;
         }
       } else {
         /* wrong data item detected*/
         OPENER_TRACE_ERR(
           "notifyCPF: got something besides the expected CIP_ITEM_ID_UNCONNECTEDMESSAGE\n");
         GenerateEncapsulationHeader(received_data,
-                                    return_value,
+                                    0,
                                     received_data->session_handle,
                                     kEncapsulationProtocolIncorrectData,
                                     outgoing_message);
-        return_value = outgoing_message->used_message_length;
+        return_value = kEipStatusOkSend;
       }
     } else {
       OPENER_TRACE_ERR(
         "notifyCPF: got something besides the expected CIP_ITEM_ID_NULL\n");
       GenerateEncapsulationHeader(received_data,
-                                  return_value,
+                                  0,
                                   received_data->session_handle,
                                   kEncapsulationProtocolIncorrectData,
                                   outgoing_message);
-      return_value = outgoing_message->used_message_length;
+      return_value = kEipStatusOkSend;
     }
   }
   return return_value;
 }
 
-int NotifyConnectedCommonPacketFormat(
+EipStatus NotifyConnectedCommonPacketFormat(
   const EncapsulationData *const received_data,
   const struct sockaddr *const originator_address,
   ENIPMessage *const outgoing_message) {
 
-  int return_value = CreateCommonPacketFormatStructure(
+  EipStatus return_value = CreateCommonPacketFormatStructure(
     received_data->current_communication_buffer_position,
     received_data->data_length, &g_common_packet_format_data_item);
 
@@ -139,7 +144,7 @@ int NotifyConnectedCommonPacketFormat(
                                         outgoing_message);
             outgoing_message->current_message_position = buffer;
             /* End regenerate encapsulation header for new message */
-            return outgoing_message->used_message_length;
+            return kEipStatusOkSend;
           }
           connection_object->sequence_count_consuming =
             g_common_packet_format_data_item.address_item.data.sequence_number;
@@ -157,7 +162,9 @@ int NotifyConnectedCommonPacketFormat(
             .connection_identifier = connection_object
                                      ->cip_produced_connection_id;
             SkipEncapsulationHeader(outgoing_message);
-            return_value = AssembleLinearMessage(
+            /* TODO: Here we lose a possible kEipStatusError from AssembleLinearMessage().
+             *  Its not clear how to transport this error information to the requester. */
+            int response_len = AssembleLinearMessage(
               &g_message_router_response, &g_common_packet_format_data_item,
               outgoing_message);
 
@@ -165,7 +172,7 @@ int NotifyConnectedCommonPacketFormat(
             outgoing_message->current_message_position =
               outgoing_message->message_buffer;
             GenerateEncapsulationHeader(received_data,
-                                        return_value,
+                                        response_len,
                                         received_data->session_handle,
                                         kEncapsulationProtocolSuccess,
                                         outgoing_message);
@@ -173,7 +180,7 @@ int NotifyConnectedCommonPacketFormat(
             memcpy(&connection_object->last_reply_sent,
                    outgoing_message,
                    sizeof(ENIPMessage) );
-            return_value = outgoing_message->used_message_length;
+            return_value = kEipStatusOkSend;
           }
         } else {
           /* wrong data item detected*/
@@ -189,7 +196,8 @@ int NotifyConnectedCommonPacketFormat(
         "notifyConnectedCPF: got something besides the expected CIP_ITEM_ID_NULL\n");
     }
   }
-  return outgoing_message->used_message_length;
+  // return outgoing_message->used_message_length;
+  return (0 != outgoing_message->used_message_length ? kEipStatusOkSend : kEipStatusOk); /* TODO: What would the right EipStatus to return? */
 }
 
 /**

source/src/enet_encap/cpf.h

@@ -78,11 +78,12 @@ typedef struct {
  * @param received_data pointer to the encapsulation structure with the received message
  * @param originator_address Address struct of the originator
  * @param outgoing_message The outgoing ENIP message struct
- * @return number of bytes to be sent back. < 0 if nothing should be sent
+ * @return kEipStatusOkSend: a response needs to be sent, others: EIP stack status
  */
-int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
-                             const struct sockaddr *const originator_address,
-                             ENIPMessage *const outgoing_message);
+EipStatus NotifyCommonPacketFormat(
+  const EncapsulationData *const received_data,
+  const struct sockaddr *const originator_address,
+  ENIPMessage *const outgoing_message);
 
 /** @ingroup ENCAP
  * Parse the CPF data from a received connected explicit message, check
@@ -92,9 +93,9 @@ int NotifyCommonPacketFormat(const EncapsulationData *const received_data,
  * @param received_data pointer to the encapsulation structure with the received message
  * @param originator_address Address struct of the originator
  * @param outgoing_message The outgoing ENIP message struct
- * @return number of bytes to be sent back. < 0 if nothing should be sent
+ * @return kEipStatusOkSend: a response needs to be sent, others: EIP stack status
  */
-int NotifyConnectedCommonPacketFormat(
+EipStatus NotifyConnectedCommonPacketFormat(
   const EncapsulationData *const received_data,
   const struct sockaddr *const originator_address,
   ENIPMessage *const outgoing_message);
@@ -116,9 +117,9 @@ EipStatus CreateCommonPacketFormatStructure(
 /** @ingroup ENCAP
  * Copy data from CPFDataItem into linear memory in message for transmission over in encapsulation.
  * @param  common_packet_format_data_item pointer to CPF structure which has to be aligned into linear memory.
- * @param  message Modified ENIP message struct
+ * @param  message Modified ENIPMessage struct
  * @return length of modification in bytes
- *     EIP_ERROR .. error
+ *     kEipStatusError .. error
  */
 int AssembleIOMessage(
   const CipCommonPacketFormatData *const common_packet_format_data_item,
@@ -131,7 +132,7 @@ int AssembleIOMessage(
  *
  * @param  message_router_response	pointer to message router response which has to be aligned into linear memory.
  * @param  common_packet_format_data_item	pointer to CPF structure which has to be aligned into linear memory.
- * @param  outgoing_message Modified ENIP message struct
+ * @param  outgoing_message Modified ENIPMessage struct
  * @return length of modification in bytes
  *         kEipStatusError .. error
  */

source/src/enet_encap/encap.c

@@ -150,12 +150,15 @@ void EncapsulationInit(void) {
             "Communications" );
 }
 
-int HandleReceivedExplictTcpData(int socket,
-                                 EipUint8 *buffer,
-                                 size_t length,
-                                 int *remaining_bytes,
-                                 struct sockaddr *originator_address,
-                                 ENIPMessage *const outgoing_message) {
+EipStatus HandleReceivedExplictTcpData
+(
+  int socket,
+  EipUint8 *buffer,
+  size_t length,
+  int *remaining_bytes,
+  struct sockaddr *originator_address,
+  ENIPMessage *const outgoing_message)
+{
   OPENER_TRACE_INFO("Handles data for TCP socket: %d\n", socket);
   EipStatus return_value = kEipStatusOk;
   EncapsulationData encapsulation_data = { 0 };
@@ -234,14 +237,17 @@ int HandleReceivedExplictTcpData(int socket,
   return return_value;
 }
 
-int HandleReceivedExplictUdpData(const int socket,
-                                 const struct sockaddr_in *from_address,
-                                 const EipUint8 *buffer,
-                                 const size_t buffer_length,
-                                 int *number_of_remaining_bytes,
-                                 bool unicast,
-                                 ENIPMessage *const outgoing_message) {
-  EipStatus status = kEipStatusOk;
+EipStatus HandleReceivedExplictUdpData
+(
+  const int socket,
+  const struct sockaddr_in *from_address,
+  const EipUint8 *buffer,
+  const size_t buffer_length,
+  int *number_of_remaining_bytes,
+  bool unicast,
+  ENIPMessage *const outgoing_message)
+{
+  EipStatus return_value = kEipStatusOk;
   EncapsulationData encapsulation_data = { 0 };
   /* eat the encapsulation header*/
   /* the structure contains a pointer to the encapsulated data*/
@@ -256,7 +262,7 @@ int HandleReceivedExplictUdpData(const int socket,
     {
       /* full package or more received */
       encapsulation_data.status = kEncapsulationProtocolSuccess;
-      status = kEipStatusOkSend;
+      return_value = kEipStatusOkSend;
       /* most of these functions need a reply to be send */
       switch (encapsulation_data.command_code) {
         case (kEncapsulationCommandListServices):
@@ -275,8 +281,9 @@ int HandleReceivedExplictUdpData(const int socket,
                                                  from_address,
                                                  &encapsulation_data,
                                                  outgoing_message);
-            status = kEipStatusOk;
-          }                       /* as the response has to be delayed do not send it now */
+            /* as the response has to be delayed do not send it now */
+            return_value = kEipStatusOk;
+          }
           break;
 
         case (kEncapsulationCommandListInterfaces):
@@ -299,14 +306,9 @@ int HandleReceivedExplictUdpData(const int socket,
           encapsulation_data.data_length = 0;
           break;
       }
-
-      if (kEipStatusOk < status) {
-        /* if status is greater than 0 data has to be sent */
-        //status = EncapsulateData(&encapsulation_data);
-      }
     }
   }
-  return outgoing_message->used_message_length;
+  return return_value;
 }
 
 void SkipEncapsulationHeader(ENIPMessage *const outgoing_message) {
@@ -613,11 +615,13 @@ void HandleReceivedRegisterSessionCommand(int socket,
 
 }
 
-/*   TODO: Update and doxyfy
- * INT8 UnregisterSession(struct S_Encapsulation_Data *pa_S_ReceiveData)
- *   close all corresponding TCP connections and delete session handle.
- *      pa_S_ReceiveData pointer to unregister session request with corresponding socket handle.
- */
+/** @brief Unregister encapsulation session
+* @param receive_data Pointer to structure with data and header information.
+* @param outgoing_message The outgoing ENIP message
+* @return kEipStatusOkSend: a response needs to be sent, others: EIP stack status
+*
+* Close all corresponding TCP connections and delete session handle.
+*/
 EipStatus HandleReceivedUnregisterSessionCommand(
   const EncapsulationData *const receive_data,
   ENIPMessage *const outgoing_message) {
@@ -653,6 +657,7 @@ EipStatus HandleReceivedSendUnitDataCommand(
   const struct sockaddr *const originator_address,
   ENIPMessage *const outgoing_message) {
   EipStatus return_value = kEipStatusOkSend;
+  /*EipStatus*/ return_value = kEipStatusOk;    /* TODO: Shouldn't this be kEipStatusOk cause we must not send any response if data_length < 6? */
 
   if (receive_data->data_length >= 6) {
     /* Command specific data UDINT .. Interface Handle, UINT .. Timeout, CPF packets */
@@ -665,16 +670,9 @@ EipStatus HandleReceivedSendUnitDataCommand(
 
     if (kSessionStatusValid == CheckRegisteredSessions(receive_data) )            /* see if the EIP session is registered*/
     {
-      EipInt16 send_size =
-        NotifyConnectedCommonPacketFormat(receive_data,
-                                          originator_address,
-                                          outgoing_message);
-
-      return_value = send_size;
-
-      if (send_size < 0) {                   /* need to send reply */
-        return_value = kEipStatusError;
-      }
+      return_value = NotifyConnectedCommonPacketFormat(receive_data,
+                                                       originator_address,
+                                                       outgoing_message);
     } else {             /* received a package with non registered session handle */
       InitializeENIPMessage(outgoing_message);
       GenerateEncapsulationHeader(receive_data,
@@ -682,6 +680,7 @@ EipStatus HandleReceivedSendUnitDataCommand(
                                   receive_data->session_handle,
                                   kEncapsulationProtocolInvalidSessionHandle,
                                   outgoing_message);
+      return_value = kEipStatusOkSend;  /* TODO: Needs to be here if line with first TODO of this function is adjusted. */
     }
   }
   return return_value;
@@ -692,6 +691,7 @@ EipStatus HandleReceivedSendUnitDataCommand(
  *  @param originator_address Address of the originator as received from socket
  *  @param outgoing_message The outgoing ENIP message
  *  @return status      kEipStatusOk .. success.
+ *                      kEipStatusOkSend .. success & need to send response
  *                      kEipStatusError .. error
  */
 EipStatus HandleReceivedSendRequestResponseDataCommand(
@@ -699,6 +699,7 @@ EipStatus HandleReceivedSendRequestResponseDataCommand(
   const struct sockaddr *const originator_address,
   ENIPMessage *const outgoing_message) {
   EipStatus return_value = kEipStatusOkSend;
+  /* EipStatus*/ return_value = kEipStatusOk;   /* TODO: Shouldn't this be kEipStatusOk cause we must not send any response if data_length < 6? */
 
   if (receive_data->data_length >= 6) {
     /* Command specific data UDINT .. Interface Handle, UINT .. Timeout, CPF packets */
@@ -711,15 +712,9 @@ EipStatus HandleReceivedSendRequestResponseDataCommand(
 
     if (kSessionStatusValid == CheckRegisteredSessions(receive_data) )            /* see if the EIP session is registered*/
     {
-      EipInt16 send_size =
-        NotifyCommonPacketFormat(receive_data,
-                                 originator_address,
-                                 outgoing_message);
-      return_value = send_size;
-
-      if (send_size < 0) {                   /* need to send reply */
-        return_value = kEipStatusError;
-      }
+      return_value = NotifyCommonPacketFormat(receive_data,
+                                              originator_address,
+                                              outgoing_message);
     } else {             /* received a package with non registered session handle */
       InitializeENIPMessage(outgoing_message);
       GenerateEncapsulationHeader(receive_data,
@@ -727,6 +722,7 @@ EipStatus HandleReceivedSendRequestResponseDataCommand(
                                   receive_data->session_handle,
                                   kEncapsulationProtocolInvalidSessionHandle,
                                   outgoing_message);
+      return_value = kEipStatusOkSend;  /* TODO: Needs to be here if line with first TODO of this function is adjusted. */
     }
   }
   return return_value;
@@ -742,7 +738,7 @@ EipStatus HandleReceivedInvalidCommand(
                               receive_data->session_handle,
                               kEncapsulationProtocolInvalidCommand,
                               outgoing_message);
-  return outgoing_message->used_message_length;
+  return kEipStatusOkSend;
 
 }
 

source/src/opener_api.h

@@ -156,8 +156,8 @@ CipClass *CreateCipClass( const EipUint32 class_id,
 /** @ingroup CIP_API
  * @brief Add a number of CIP instances to a given CIP class
  *
- * The required number of instances are created in a block, but are attached to
- * the class as a linked list.
+ * The required number of instances are attached to the class as a linked list.
+ *
  * The instances are numbered sequentially -- i.e. the first node in the chain
  * is instance 1, the second is 2, and so on.
  * You can add new instances at any time (you do not have to create all the
@@ -188,6 +188,7 @@ CipInstance *AddCipInstances(
 CipInstance *AddCIPInstance(CipClass *RESTRICT const cip_class_to_add_instance,
                             const EipUint32 instance_id);
 
+
 /** @ingroup CIP_API
  * @brief Insert an attribute in an instance of a CIP class
  *
@@ -201,14 +202,13 @@ CipInstance *AddCIPInstance(CipClass *RESTRICT const cip_class_to_add_instance,
  *  @param cip_data Pointer to data of attribute.
  *  @param cip_flags Flags to indicate set-ability and get-ability of attribute.
  */
-
-
-
 void InsertAttribute(CipInstance *const cip_instance,
                      const EipUint16 attribute_number,
                      const EipUint8 cip_data_type,
                      void *const cip_data,
                      const EipByte cip_flags);
+
+
 /** @ingroup CIP_API
  * @brief Allocates Attribute bitmasks
  *
@@ -433,14 +433,17 @@ void ConfigureListenOnlyConnectionPoint(
  * over after we're done here
  * @param originator_address Address struct of the message originator
  * @param outgoing_message The outgoing ENIP message
- * @return length of reply that need to be sent back
+ * @return kEipStatusOkSend: a response needs to be sent, others: EIP stack status
  */
-int HandleReceivedExplictTcpData(int socket_handle,
-                                 EipUint8 *buffer,
-                                 size_t length,
-                                 int *number_of_remaining_bytes,
-                                 struct sockaddr *originator_address,
-                                 ENIPMessage *const outgoing_message);
+EipStatus HandleReceivedExplictTcpData
+(
+  int socket_handle,
+  EipUint8 *buffer,
+  size_t length,
+  int *number_of_remaining_bytes,
+  struct sockaddr *originator_address,
+  ENIPMessage *const outgoing_message);
+
 
 /** @ingroup CIP_API
  * @brief Notify the encapsulation layer that an explicit message has been
@@ -453,17 +456,20 @@ int HandleReceivedExplictTcpData(int socket_handle,
  * @param buffer_length length of the data in buffer.
  * @param number_of_remaining_bytes return how many bytes of the input are left
  * over after we're done here
- * @param unicast Was the data receieved from a multicast address
+ * @param unicast Was the data received as unicast message?
  * @param outgoing_message Outgoing ENIP message
- * @return length of reply that need to be sent back
- */
-int HandleReceivedExplictUdpData(const int socket_handle,
-                                 const struct sockaddr_in *from_address,
-                                 const EipUint8 *buffer,
-                                 const size_t buffer_length,
-                                 int *number_of_remaining_bytes,
-                                 bool unicast,
-                                 ENIPMessage *const outgoing_message);
+ * @return kEipStatusOkSend: a response needs to be sent, others: EIP stack status
+ */
+EipStatus HandleReceivedExplictUdpData
+(
+  const int socket_handle,
+  const struct sockaddr_in *from_address,
+  const EipUint8 *buffer,
+  const size_t buffer_length,
+  int *number_of_remaining_bytes,
+  bool unicast,
+  ENIPMessage *const outgoing_message);
+
 
 /** @ingroup CIP_API
  *  @brief Notify the connection manager that data for a connection has been

source/src/ports/generic_networkhandler.c

@@ -493,7 +493,7 @@ void CheckAndHandleUdpGlobalBroadcastSocket(void) {
     do {
       ENIPMessage outgoing_message;
       InitializeENIPMessage(&outgoing_message);
-      int reply_length = HandleReceivedExplictUdpData(
+      EipStatus need_to_send = HandleReceivedExplictUdpData(
         g_network_status.udp_global_broadcast_listener,
         &from_address,
         receive_buffer,
@@ -505,7 +505,7 @@ void CheckAndHandleUdpGlobalBroadcastSocket(void) {
       receive_buffer += received_size - remaining_bytes;
       received_size = remaining_bytes;
 
-      if (reply_length > 0) {
+      if (need_to_send > 0) {
         OPENER_TRACE_INFO("UDP broadcast reply sent:\n");
 
         /* if the active socket matches a registered UDP callback, handle a UDP packet */
@@ -513,7 +513,7 @@ void CheckAndHandleUdpGlobalBroadcastSocket(void) {
                     (char *) outgoing_message.message_buffer,
                     outgoing_message.used_message_length, 0,
                     (struct sockaddr *) &from_address, sizeof(from_address) )
-            != reply_length) {
+            != outgoing_message.used_message_length) {
           OPENER_TRACE_INFO(
             "networkhandler: UDP response was not fully sent\n");
         }
@@ -558,14 +558,14 @@ void CheckAndHandleUdpUnicastSocket(void) {
     ENIPMessage outgoing_message;
     InitializeENIPMessage(&outgoing_message);
     do {
-      int reply_length = HandleReceivedExplictUdpData(
+      EipStatus need_to_send = HandleReceivedExplictUdpData(
         g_network_status.udp_unicast_listener, &from_address, receive_buffer,
         received_size, &remaining_bytes, true, &outgoing_message);
 
       receive_buffer += received_size - remaining_bytes;
       received_size = remaining_bytes;
 
-      if (reply_length > 0) {
+      if (need_to_send > 0) {
         OPENER_TRACE_INFO("UDP unicast reply sent:\n");
 
         /* if the active socket matches a registered UDP callback, handle a UDP packet */
@@ -573,7 +573,7 @@ void CheckAndHandleUdpUnicastSocket(void) {
                     (char *) outgoing_message.message_buffer,
                     outgoing_message.used_message_length, 0,
                     (struct sockaddr *) &from_address, sizeof(from_address) )
-            != reply_length) {
+            != outgoing_message.used_message_length) {
           OPENER_TRACE_INFO(
             "networkhandler: UDP unicast response was not fully sent\n");
         }
@@ -786,7 +786,7 @@ EipStatus HandleDataOnTcpSocket(int socket) {
 
     ENIPMessage outgoing_message = {0};
     InitializeENIPMessage(&outgoing_message);
-    int need_to_send = HandleReceivedExplictTcpData(
+    EipStatus need_to_send = HandleReceivedExplictTcpData(
       socket, incoming_message, data_size, &remaining_bytes,
       &sender_address, &outgoing_message);
     SocketTimer *socket_timer = SocketTimerArrayGetSocketTimer(