Updated rmw_* return codes (#443)

* Updated rmw_*_*_allocation return codes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed RMW_RET_INCORRECT_RMW_IMPLEMENTATION return codes Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved return error in rmw_wait Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed include Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved wait_set return codes Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Removed warning Signed-off-by: ahcorde <ahcorde@gmail.com> * make linters happy Signed-off-by: ahcorde <ahcorde@gmail.com>
ros2 · Sep 25, 2020 · 867514b · 867514b
1 parent f03bee2
commit 867514b
Show file tree

Hide file tree

Showing 10 changed files with 37 additions and 40 deletions.
diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp
@@ -46,7 +46,7 @@ rmw_init_publisher_allocation(
   (void) message_bounds;
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_ret_t
@@ -55,7 +55,7 @@ rmw_fini_publisher_allocation(rmw_publisher_allocation_t * allocation)
   // Unused in current implementation.
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_publisher_t *

diff --git a/rmw_fastrtps_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_cpp/src/rmw_subscription.cpp
@@ -42,7 +42,7 @@ rmw_init_subscription_allocation(
   (void) message_bounds;
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_ret_t
@@ -51,7 +51,7 @@ rmw_fini_subscription_allocation(rmw_subscription_allocation_t * allocation)
   // Unused in current implementation.
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_subscription_t *

diff --git a/rmw_fastrtps_cpp/src/rmw_wait.cpp b/rmw_fastrtps_cpp/src/rmw_wait.cpp
@@ -16,6 +16,7 @@
 #include "rmw/rmw.h"
 
 #include "rmw_fastrtps_shared_cpp/rmw_common.hpp"
+#include "rmw_fastrtps_cpp/identifier.hpp"
 
 extern "C"
 {
@@ -30,6 +31,7 @@ rmw_wait(
   const rmw_time_t * wait_timeout)
 {
   return rmw_fastrtps_shared_cpp::__rmw_wait(
-    subscriptions, guard_conditions, services, clients, events, wait_set, wait_timeout);
+    eprosima_fastrtps_identifier, subscriptions, guard_conditions, services, clients, events,
+    wait_set, wait_timeout);
 }
 }  // extern "C"
diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp
@@ -48,7 +48,7 @@ rmw_init_publisher_allocation(
   (void) message_bounds;
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_ret_t
@@ -57,7 +57,7 @@ rmw_fini_publisher_allocation(rmw_publisher_allocation_t * allocation)
   // Unused in current implementation.
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_publisher_t *

diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp
@@ -44,7 +44,7 @@ rmw_init_subscription_allocation(
   (void) message_bounds;
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_ret_t
@@ -53,7 +53,7 @@ rmw_fini_subscription_allocation(rmw_subscription_allocation_t * allocation)
   // Unused in current implementation.
   (void) allocation;
   RMW_SET_ERROR_MSG("unimplemented");
-  return RMW_RET_ERROR;
+  return RMW_RET_UNSUPPORTED;
 }
 
 rmw_subscription_t *

diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_wait.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_wait.cpp
@@ -16,6 +16,7 @@
 #include "rmw/rmw.h"
 
 #include "rmw_fastrtps_shared_cpp/rmw_common.hpp"
+#include "rmw_fastrtps_dynamic_cpp/identifier.hpp"
 
 extern "C"
 {
@@ -30,6 +31,7 @@ rmw_wait(
   const rmw_time_t * wait_timeout)
 {
   return rmw_fastrtps_shared_cpp::__rmw_wait(
-    subscriptions, guard_conditions, services, clients, events, wait_set, wait_timeout);
+    eprosima_fastrtps_identifier, subscriptions, guard_conditions, services, clients, events,
+    wait_set, wait_timeout);
 }
 }  // extern "C"
diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp
@@ -355,6 +355,7 @@ __rmw_get_topic_names_and_types(
 RMW_FASTRTPS_SHARED_CPP_PUBLIC
 rmw_ret_t
 __rmw_wait(
+  const char * identifier,
   rmw_subscriptions_t * subscriptions,
   rmw_guard_conditions_t * guard_conditions,
   rmw_services_t * services,

diff --git a/rmw_fastrtps_shared_cpp/src/listener_thread.cpp b/rmw_fastrtps_shared_cpp/src/listener_thread.cpp
@@ -136,6 +136,7 @@ node_listener(rmw_context_t * context)
       TERMINATE_THREAD("failed to create wait set");
     }
     if (RMW_RET_OK != rmw_fastrtps_shared_cpp::__rmw_wait(
+        context->implementation_identifier,
         &subscriptions,
         &guard_conditions,
         nullptr,

diff --git a/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp b/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp
@@ -90,6 +90,7 @@ namespace rmw_fastrtps_shared_cpp
 {
 rmw_ret_t
 __rmw_wait(
+  const char * identifier,
   rmw_subscriptions_t * subscriptions,
   rmw_guard_conditions_t * guard_conditions,
   rmw_services_t * services,
@@ -98,25 +99,21 @@ __rmw_wait(
   rmw_wait_set_t * wait_set,
   const rmw_time_t * wait_timeout)
 {
-  if (!wait_set) {
-    RMW_SET_ERROR_MSG("wait set handle is null");
-    return RMW_RET_ERROR;
-  }
+  RMW_CHECK_ARGUMENT_FOR_NULL(wait_set, RMW_RET_INVALID_ARGUMENT);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    wait set handle,
+    wait_set->implementation_identifier, identifier,
+    return RMW_RET_INCORRECT_RMW_IMPLEMENTATION)
+
+  // If wait_set_info is ever nullptr, it can only mean one of three things:
+  // - Wait set is invalid. Caller did not respect preconditions.
+  // - Implementation is logically broken. Definitely not something we want to treat as a normal
+  // error.
+  // - Heap is corrupt.
+  // In all three cases, it's better if this crashes soon enough.
   CustomWaitsetInfo * wait_set_info = static_cast<CustomWaitsetInfo *>(wait_set->data);
-  if (!wait_set_info) {
-    RMW_SET_ERROR_MSG("Waitset info struct is null");
-    return RMW_RET_ERROR;
-  }
   std::mutex * conditionMutex = &wait_set_info->condition_mutex;
   std::condition_variable * conditionVariable = &wait_set_info->condition;
-  if (!conditionMutex) {
-    RMW_SET_ERROR_MSG("Mutex for wait set was null");
-    return RMW_RET_ERROR;
-  }
-  if (!conditionVariable) {
-    RMW_SET_ERROR_MSG("Condition variable for wait set was null");
-    return RMW_RET_ERROR;
-  }
 
   if (subscriptions) {
     for (size_t i = 0; i < subscriptions->subscriber_count; ++i) {

diff --git a/rmw_fastrtps_shared_cpp/src/rmw_wait_set.cpp b/rmw_fastrtps_shared_cpp/src/rmw_wait_set.cpp
@@ -73,26 +73,20 @@ __rmw_create_wait_set(const char * identifier, rmw_context_t * context, size_t m
 rmw_ret_t
 __rmw_destroy_wait_set(const char * identifier, rmw_wait_set_t * wait_set)
 {
-  if (!wait_set) {
-    RMW_SET_ERROR_MSG("wait set handle is null");
-    return RMW_RET_ERROR;
-  }
+  RMW_CHECK_ARGUMENT_FOR_NULL(wait_set, RMW_RET_ERROR);
   RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
     wait set handle,
     wait_set->implementation_identifier, identifier,
-    return RMW_RET_ERROR)
+    return RMW_RET_INCORRECT_RMW_IMPLEMENTATION)
 
   auto result = RMW_RET_OK;
+  // If wait_set_info is ever nullptr, it can only mean one of three things:
+  // - Wait set is invalid. Caller did not respect preconditions.
+  // - Implementation is logically broken. Definitely not something we want to treat as a normal
+  // error.
+  // - Heap is corrupt.
+  // In all three cases, it's better if this crashes soon enough.
   auto wait_set_info = static_cast<CustomWaitsetInfo *>(wait_set->data);
-  if (!wait_set_info) {
-    RMW_SET_ERROR_MSG("wait set info is null");
-    return RMW_RET_ERROR;
-  }
-  std::mutex * conditionMutex = &wait_set_info->condition_mutex;
-  if (!conditionMutex) {
-    RMW_SET_ERROR_MSG("wait set mutex is null");
-    return RMW_RET_ERROR;
-  }
 
   if (wait_set->data) {
     if (wait_set_info) {