expand rcl_node_is_valid with allocator to standarize the node check … (

#173)

* expand rcl_node_is_valid with allocator to standarize the node check across RCL

Current RCL use very different way for node check in different components. Actually,
node.c already provide a standard way - rcl_node_is_valid. However, it just use
internal allocator, but some context has external allocator. The commit expand
rcl_node_is_valid with one more parameter for allocator. It can be used in all cases.

Signed-off-by: jwang <jing.j.wang@intel.com>

* Add allocator check in rcl_node_is_valid

* Separate argument checks from the concept of what makes a node invalid

* clarify allocator only used for error messages; default if NULL

* shorten parameter description

rcl/include/rcl/client.h

@@ -177,6 +177,7 @@ rcl_client_init(
  * \param[in] node handle to the node used to create the client
  * \return `RCL_RET_OK` if client was finalized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
 RCL_PUBLIC

rcl/include/rcl/node.h

@@ -203,7 +203,11 @@ rcl_node_get_default_options(void);
 
 /// Return `true` if the node is valid, else `false`.
 /**
- * Also return `false` if the node pointer is `NULL`.
+ * Also return `false` if the node pointer is `NULL` or the allocator is invalid.
+ *
+ * The allocator needs to either be a valid allocator or `NULL`, in which case
+ * the default allocator will be used.
+ * The allocator is used when allocation is needed for an error message.
  *
  * A node is invalid if:
  *   - the implementation is `NULL` (rcl_node_init not called or failed)
@@ -215,7 +219,7 @@ rcl_node_get_default_options(void);
  * Consider:
  *
  * ```c
- * assert(rcl_node_is_valid(node));  // <-- thread 1
+ * assert(rcl_node_is_valid(node, NULL));  // <-- thread 1
  * rcl_shutdown();                   // <-- thread 2
  * // use node as if valid           // <-- thread 1
  * ```
@@ -234,11 +238,12 @@ rcl_node_get_default_options(void);
  * <i>[1] if `atomic_is_lock_free()` returns true for `atomic_uint_least64_t`</i>
  *
  * \param[in] node rcl_node_t to be validated
- * \return `true` if the node is valid, otherwise `false`.
+ * \param[in] allocator a valid allocator or `NULL`
+ * \return `true` if the node and allocator are valid, otherwise `false`.
  */
 RCL_PUBLIC
 bool
-rcl_node_is_valid(const rcl_node_t * node);
+rcl_node_is_valid(const rcl_node_t * node, const rcl_allocator_t * allocator);
 
 /// Return the name of the node.
 /**

rcl/include/rcl/publisher.h

@@ -171,6 +171,7 @@ rcl_publisher_init(
  * \param[in] node handle to the node used to create the publisher
  * \return `RCL_RET_OK` if publisher was finalized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
 RCL_PUBLIC

rcl/include/rcl/service.h

@@ -138,6 +138,7 @@ rcl_get_zero_initialized_service(void);
  * \param[in] options service options, including quality of service settings
  * \return `RCL_RET_OK` if service was initialized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_SERVICE_NAME_INVALID` if the given service name is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
@@ -174,6 +175,7 @@ rcl_service_init(
  * \param[in] node handle to the node used to create the service
  * \return `RCL_RET_OK` if service was deinitialized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
 RCL_PUBLIC

rcl/include/rcl/subscription.h

@@ -140,6 +140,7 @@ rcl_get_zero_initialized_subscription(void);
  * \param[in] options subscription options, including quality of service settings
  * \return `RCL_RET_OK` if subscription was initialized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
@@ -176,6 +177,7 @@ rcl_subscription_init(
  * \param[in] node handle to the node used to create the subscription
  * \return `RCL_RET_OK` if subscription was deinitialized successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_NODE_INVALID` if the node is invalid, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
 RCL_PUBLIC

rcl/src/rcl/client.c

@@ -62,8 +62,7 @@ rcl_client_init(
   RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT, *allocator);
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
-  if (!node->impl) {
-    RCL_SET_ERROR_MSG("invalid node", *allocator);
+  if (!rcl_node_is_valid(node, allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT, *allocator);
@@ -167,6 +166,9 @@ rcl_client_fini(rcl_client_t * client, rcl_node_t * node)
   rcl_ret_t result = RCL_RET_OK;
   RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
+  if (!rcl_node_is_valid(node, NULL)) {
+    return RCL_RET_NODE_INVALID;
+  }
   if (client->impl) {
     rcl_allocator_t allocator = client->impl->options.allocator;
     rmw_node_t * rmw_node = rcl_node_get_rmw_handle(node);

rcl/src/rcl/graph.c

@@ -37,8 +37,8 @@ rcl_get_topic_names_and_types(
   rcl_names_and_types_t * topic_names_and_types)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator())
-  RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator)
-  if (!rcl_node_is_valid(node)) {
+  RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
+  if (!rcl_node_is_valid(node, allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(topic_names_and_types, RCL_RET_INVALID_ARGUMENT, *allocator)
@@ -64,7 +64,7 @@ rcl_get_service_names_and_types(
   rcl_names_and_types_t * service_names_and_types)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(service_names_and_types, RCL_RET_INVALID_ARGUMENT, *allocator);
@@ -98,7 +98,7 @@ rcl_get_node_names(
   rcutils_string_array_t * node_names)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, allocator);
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, &allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(node_names, RCL_RET_INVALID_ARGUMENT, allocator);
@@ -123,7 +123,7 @@ rcl_count_publishers(
   size_t * count)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return RCL_RET_NODE_INVALID;
   }
   const rcl_node_options_t * node_options = rcl_node_get_options(node);
@@ -143,7 +143,7 @@ rcl_count_subscribers(
   size_t * count)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return RCL_RET_NODE_INVALID;
   }
   const rcl_node_options_t * node_options = rcl_node_get_options(node);
@@ -163,7 +163,7 @@ rcl_service_server_is_available(
   bool * is_available)
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return RCL_RET_NODE_INVALID;
   }
   const rcl_node_options_t * node_options = rcl_node_get_options(node);

rcl/src/rcl/node.c

@@ -280,10 +280,9 @@ rcl_node_init(
   node->impl->rcl_instance_id = rcl_get_instance_id();
   // graph guard condition
   rmw_graph_guard_condition = rmw_node_get_graph_guard_condition(node->impl->rmw_node_handle);
-  if (!rmw_graph_guard_condition) {
-    RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), *allocator);
-    goto fail;
-  }
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    rmw_graph_guard_condition, rmw_get_error_string_safe(), goto fail, *allocator);
+
   node->impl->graph_guard_condition = (rcl_guard_condition_t *)allocator->allocate(
     sizeof(rcl_guard_condition_t), allocator->state);
   RCL_CHECK_FOR_NULL_WITH_MSG(
@@ -363,14 +362,16 @@ rcl_node_fini(rcl_node_t * node)
 }
 
 bool
-rcl_node_is_valid(const rcl_node_t * node)
+rcl_node_is_valid(const rcl_node_t * node, const rcl_allocator_t * allocator)
 {
-  RCL_CHECK_ARGUMENT_FOR_NULL(node, false, rcl_get_default_allocator());
+  rcl_allocator_t alloc = allocator ? *allocator : rcl_get_default_allocator();
+  RCL_CHECK_ALLOCATOR_WITH_MSG(&alloc, "invalid allocator", return false);
+  RCL_CHECK_ARGUMENT_FOR_NULL(node, false, alloc);
   RCL_CHECK_FOR_NULL_WITH_MSG(
-    node->impl, "rcl node implementation is invalid", return false, rcl_get_default_allocator());
+    node->impl, "rcl node implementation is invalid", return false, alloc);
   if (node->impl->rcl_instance_id != rcl_get_instance_id()) {
     RCL_SET_ERROR_MSG(
-      "rcl node is invalid, rcl instance id does not match", rcl_get_default_allocator());
+      "rcl node is invalid, rcl instance id does not match", alloc);
     return false;
   }
   return true;
@@ -391,7 +392,7 @@ rcl_node_get_default_options()
 const char *
 rcl_node_get_name(const rcl_node_t * node)
 {
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return NULL;
   }
   return node->impl->rmw_node_handle->name;
@@ -400,7 +401,7 @@ rcl_node_get_name(const rcl_node_t * node)
 const char *
 rcl_node_get_namespace(const rcl_node_t * node)
 {
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return NULL;
   }
   return node->impl->rmw_node_handle->namespace_;
@@ -409,7 +410,7 @@ rcl_node_get_namespace(const rcl_node_t * node)
 const rcl_node_options_t *
 rcl_node_get_options(const rcl_node_t * node)
 {
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return NULL;
   }
   return &node->impl->options;
@@ -431,7 +432,7 @@ rcl_node_get_domain_id(const rcl_node_t * node, size_t * domain_id)
 rmw_node_t *
 rcl_node_get_rmw_handle(const rcl_node_t * node)
 {
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return NULL;
   }
   return node->impl->rmw_node_handle;
@@ -449,7 +450,7 @@ rcl_node_get_rcl_instance_id(const rcl_node_t * node)
 const struct rcl_guard_condition_t *
 rcl_node_get_graph_guard_condition(const rcl_node_t * node)
 {
-  if (!rcl_node_is_valid(node)) {
+  if (!rcl_node_is_valid(node, NULL)) {
     return NULL;
   }
   return node->impl->graph_guard_condition;

rcl/src/rcl/publisher.c

@@ -61,12 +61,11 @@ rcl_publisher_init(
   RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_INVALID_ARGUMENT, *allocator);
   if (publisher->impl) {
     RCL_SET_ERROR_MSG(
-      "publisher already initialized, or memory was unintialized", rcl_get_default_allocator());
+      "publisher already initialized, or memory was unintialized", *allocator);
     return RCL_RET_ALREADY_INIT;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
-  if (!node->impl) {
-    RCL_SET_ERROR_MSG("invalid node", *allocator);
+  if (!rcl_node_is_valid(node, allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT, *allocator);
@@ -145,10 +144,8 @@ rcl_publisher_init(
     expanded_topic_name,
     &(options->qos));
   allocator->deallocate(expanded_topic_name, allocator->state);
-  if (!publisher->impl->rmw_handle) {
-    RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), *allocator);
-    goto fail;
-  }
+  RCL_CHECK_FOR_NULL_WITH_MSG(publisher->impl->rmw_handle,
+    rmw_get_error_string_safe(), goto fail, *allocator);
   // options
   publisher->impl->options = *options;
   return RCL_RET_OK;
@@ -165,6 +162,9 @@ rcl_publisher_fini(rcl_publisher_t * publisher, rcl_node_t * node)
   rcl_ret_t result = RCL_RET_OK;
   RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
+  if (!rcl_node_is_valid(node, NULL)) {
+    return RCL_RET_NODE_INVALID;
+  }
   if (publisher->impl) {
     rcl_allocator_t allocator = publisher->impl->options.allocator;
     rmw_node_t * rmw_node = rcl_node_get_rmw_handle(node);

rcl/src/rcl/service.c

@@ -59,8 +59,7 @@ rcl_service_init(
 
   RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT, *allocator);
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
-  if (!node->impl) {
-    RCL_SET_ERROR_MSG("invalid node", *allocator);
+  if (!rcl_node_is_valid(node, allocator)) {
     return RCL_RET_NODE_INVALID;
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT, *allocator);
@@ -170,6 +169,9 @@ rcl_service_fini(rcl_service_t * service, rcl_node_t * node)
   rcl_ret_t result = RCL_RET_OK;
   RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
+  if (!rcl_node_is_valid(node, NULL)) {
+    return RCL_RET_NODE_INVALID;
+  }
   if (service->impl) {
     rcl_allocator_t allocator = service->impl->options.allocator;
     rmw_node_t * rmw_node = rcl_node_get_rmw_handle(node);

rcl/src/rcl/subscription.c

@@ -55,9 +55,12 @@ rcl_subscription_init(
   RCL_CHECK_ARGUMENT_FOR_NULL(options, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
   const rcl_allocator_t * allocator = &options->allocator;
   RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
-
   RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT, *allocator);
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, *allocator);
+  if (!rcl_node_is_valid(node, allocator)) {
+    return RCL_RET_NODE_INVALID;
+  }
+  RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT, *allocator);
   RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT, *allocator);
   RCL_CHECK_ARGUMENT_FOR_NULL(topic_name, RCL_RET_INVALID_ARGUMENT, *allocator);
   if (subscription->impl) {
@@ -159,6 +162,9 @@ rcl_subscription_fini(rcl_subscription_t * subscription, rcl_node_t * node)
   rcl_ret_t result = RCL_RET_OK;
   RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
   RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());
+  if (!rcl_node_is_valid(node, NULL)) {
+    return RCL_RET_NODE_INVALID;
+  }
   if (subscription->impl) {
     rcl_allocator_t allocator = subscription->impl->options.allocator;
     rmw_node_t * rmw_node = rcl_node_get_rmw_handle(node);

rcl/test/rcl/test_node.cpp

@@ -118,16 +118,16 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
     });
   // Test rcl_node_is_valid().
   bool is_valid;
-  is_valid = rcl_node_is_valid(nullptr);
+  is_valid = rcl_node_is_valid(nullptr, nullptr);
   EXPECT_FALSE(is_valid);
   rcl_reset_error();
-  is_valid = rcl_node_is_valid(&zero_node);
+  is_valid = rcl_node_is_valid(&zero_node, nullptr);
   EXPECT_FALSE(is_valid);
   rcl_reset_error();
-  is_valid = rcl_node_is_valid(&invalid_node);
+  is_valid = rcl_node_is_valid(&invalid_node, nullptr);
   EXPECT_FALSE(is_valid);
   rcl_reset_error();
-  is_valid = rcl_node_is_valid(&node);
+  is_valid = rcl_node_is_valid(&node, nullptr);
   EXPECT_TRUE(is_valid);
   rcl_reset_error();
   // Test rcl_node_get_name().