From 9d9d30ecf05ddebcf536e071f76dff073ec2b713 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 13:01:26 -0300 Subject: [PATCH 01/17] Add resolve topic name Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 1 + rcl/include/rcl/expand_topic_name.h | 2 +- rcl/include/rcl/resolve_topic_name.h | 94 ++++++++++++++++++++++ rcl/src/rcl/resolve_topic_name.c | 116 +++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 rcl/include/rcl/resolve_topic_name.h create mode 100644 rcl/src/rcl/resolve_topic_name.c diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index e9e0b4c50..75662b800 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -53,6 +53,7 @@ set(${PROJECT_NAME}_sources src/rcl/node_options.c src/rcl/publisher.c src/rcl/remap.c + src/rcl/resolve_topic_name.c src/rcl/rmw_implementation_identifier_check.c src/rcl/security.c src/rcl/service.c diff --git a/rcl/include/rcl/expand_topic_name.h b/rcl/include/rcl/expand_topic_name.h index bfcc288de..4816cbfbe 100644 --- a/rcl/include/rcl/expand_topic_name.h +++ b/rcl/include/rcl/expand_topic_name.h @@ -29,7 +29,7 @@ extern "C" /// Expand a given topic name into a fully-qualified topic name. /** * The input_topic_name, node_name, and node_namespace arguments must all be - * vaid, null terminated c strings. + * valid, null terminated c strings. * The output_topic_name will not be assigned a value in the event of an error. * * The output_topic_name will be null terminated. diff --git a/rcl/include/rcl/resolve_topic_name.h b/rcl/include/rcl/resolve_topic_name.h new file mode 100644 index 000000000..b1ea08394 --- /dev/null +++ b/rcl/include/rcl/resolve_topic_name.h @@ -0,0 +1,94 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef RCL__RESOLVE_TOPIC_NAME_H_ +#define RCL__RESOLVE_TOPIC_NAME_H_ + +#ifdef __cplusplus +extern "C" +{ +#endif + +#include + +#include "rcl/allocator.h" +#include "rcl/arguments.h" +#include "rcl/macros.h" +#include "rcl/types.h" +#include "rcl/visibility_control.h" + +/// Expand a given topic name into a fully-qualified topic name and apply remapping rules. +/** + * \pre The input_topic_name, node_name, and node_namespace arguments must all be + * valid, null terminated c strings. + * \post The output_topic_name will not be assigned a value in the event of an error. + * \post If assigned, the output_topic_name will be null terminated. + * Its memory must be freed by the user. + * + * This function uses the substitutions provided by rcl_get_default_topic_name_substitutions(), + * and the additional ones explained in \ref expand_topic_name(). + * + * The remapping rules are provided by `local_args` and `global_args`, + * see \ref rcl_remap_topic_name(). + * + *
+ * Attribute | Adherence + * ------------------ | ------------- + * Allocates Memory | Yes + * Thread-Safe | Yes + * Uses Atomics | No + * Lock-Free | Yes + * + * \param[in] local_arguments Command line arguments to be used before global arguments, or + * if NULL or zero-initialized then only global arguments are used. + * \param[in] global_arguments Command line arguments to use if no local rules matched, or + * `NULL` or zero-initialized to ignore global arguments. + * \param[in] input_topic_name Topic name to be expanded and remapped. + * \param[in] node_name Name of the node associated with the topic. + * \param[in] node_namespace Namespace of the node associated with the topic. + * \param[in] allocator The allocator to be used when creating the output topic. + * \param[in] only_expand if `true`, remmapping rules aren't applied. + * \param[out] output_topic_name Output char * pointer. + * \return `RCL_RET_OK` if the topic name was expanded successfully, or + * \return `RCL_RET_INVALID_ARGUMENT` if any of input_topic_name, node_name, node_namespace + * or output_topic_name are NULL, or + * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or + * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or + * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid + * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid + * (see \ref rmw_validate_node_name()), or + * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid + * (see \ref rmw_validate_namespace()), or + * \return `RCL_RET_UNKNOWN_SUBSTITUTION` for unknown substitutions in name, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_resolve_topic_name( + const rcl_arguments_t * local_args, + const rcl_arguments_t * global_args, + const char * input_topic_name, + const char * node_name, + const char * node_namespace, + rcl_allocator_t allocator, + bool only_expand, + char ** output_topic_name); + +#ifdef __cplusplus +} +#endif + +#endif // RCL__RESOLVE_TOPIC_NAME_H_ diff --git a/rcl/src/rcl/resolve_topic_name.c b/rcl/src/rcl/resolve_topic_name.c new file mode 100644 index 000000000..6377fddd6 --- /dev/null +++ b/rcl/src/rcl/resolve_topic_name.c @@ -0,0 +1,116 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "rcl/resolve_topic_name.h" + +#include "rcutils/error_handling.h" +#include "rcutils/logging_macros.h" +#include "rcutils/types/string_map.h" + +#include "rmw/error_handling.h" +#include "rmw/validate_full_topic_name.h" + +#include "rcl/error_handling.h" +#include "rcl/expand_topic_name.h" +#include "rcl/remap.h" + +rcl_ret_t +rcl_resolve_topic_name( + const rcl_arguments_t * local_args, + const rcl_arguments_t * global_args, + const char * input_topic_name, + const char * node_name, + const char * node_namespace, + rcl_allocator_t allocator, + bool only_expand, + char ** output_topic_name) +{ + // the other arguments are checked by rcl_expand_topic_name() and rcl_remap_topic_name() + RCL_CHECK_ARGUMENT_FOR_NULL(output_topic_name, RCL_RET_INVALID_ARGUMENT); + // Create default topic name substitutions map + rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); + rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, allocator); + if (rcutils_ret != RCUTILS_RET_OK) { + const char * error = rcutils_get_error_string().str; + rcutils_reset_error(); + RCL_SET_ERROR_MSG(error); + if (RCUTILS_RET_BAD_ALLOC == rcutils_ret) { + return RCL_RET_BAD_ALLOC; + } + return RCL_RET_ERROR; + } + rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); + if (ret != RCL_RET_OK) { + if (RCL_RET_BAD_ALLOC != ret) { + ret = RCL_RET_ERROR; + } + goto cleanup; + } + // expand topic name + char * expanded_topic_name = NULL; + char * remapped_topic_name = NULL; + ret = rcl_expand_topic_name( + input_topic_name, + node_name, + node_namespace, + &substitutions_map, + allocator, + &expanded_topic_name); + if (RCL_RET_OK != ret) { + goto cleanup; + } + if (!only_expand) { + // remap topic name + ret = rcl_remap_topic_name( + local_args, global_args, expanded_topic_name, + node_name, node_namespace, allocator, &remapped_topic_name); + if (RCL_RET_OK != ret) { + goto cleanup; + } + } + if (NULL == remapped_topic_name) { + remapped_topic_name = expanded_topic_name; + expanded_topic_name = NULL; + } + // validate the result + int validation_result; + rmw_ret_t rmw_ret = rmw_validate_full_topic_name(remapped_topic_name, &validation_result, NULL); + if (rmw_ret != RMW_RET_OK) { + const char * error = rmw_get_error_string().str; + rmw_reset_error(); + RCL_SET_ERROR_MSG(error); + ret = RCL_RET_ERROR; + goto cleanup; + } + if (validation_result != RMW_TOPIC_VALID) { + RCL_SET_ERROR_MSG(rmw_full_topic_name_validation_result_string(validation_result)); + ret = RCL_RET_TOPIC_NAME_INVALID; + goto cleanup; + } + *output_topic_name = remapped_topic_name; + return RCL_RET_OK; + +cleanup: + rcutils_ret = rcutils_string_map_fini(&substitutions_map); + if (rcutils_ret != RCUTILS_RET_OK) { + RCUTILS_LOG_ERROR_NAMED( + ROS_PACKAGE_NAME, + "failed to fini string_map (%d) during error handling: %s", + rcutils_ret, + rcutils_get_error_string().str); + rcutils_reset_error(); + } + allocator.deallocate(expanded_topic_name, allocator.state); + return ret; +} From b3db324c0413eb3ce55de799b62c35b0a3e2c45e Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 13:01:47 -0300 Subject: [PATCH 02/17] Use it in subscription init Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/subscription.c | 82 ++++++++++---------------------------- 1 file changed, 20 insertions(+), 62 deletions(-) diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index e9c2748aa..e5cd9f989 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -22,8 +22,7 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/expand_topic_name.h" -#include "rcl/remap.h" +#include "rcl/resolve_topic_name.h" #include "rcutils/logging_macros.h" #include "rmw/error_handling.h" #include "rmw/validate_full_topic_name.h" @@ -67,75 +66,39 @@ rcl_subscription_init( RCL_SET_ERROR_MSG("subscription already initialized, or memory was uninitialized"); return RCL_RET_ALREADY_INIT; } - // Expand the given topic name. - rcutils_allocator_t rcutils_allocator = *allocator; // implicit conversion to rcutils version - rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); - rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, rcutils_allocator); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - if (RCUTILS_RET_BAD_ALLOC == rcutils_ret) { - return RCL_RET_BAD_ALLOC; - } + + const rcl_node_options_t * node_options = rcl_node_get_options(node); + if (NULL == node_options) { return RCL_RET_ERROR; } - rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); - if (ret != RCL_RET_OK) { - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, - "failed to fini string_map (%d) during error handling: %s", - rcutils_ret, - rcutils_get_error_string().str); - } - if (RCL_RET_BAD_ALLOC == ret) { - return ret; - } - return RCL_RET_ERROR; + rcl_arguments_t * global_args = NULL; + if (node_options->use_global_arguments) { + global_args = &(node->context->global_arguments); } - char * expanded_topic_name = NULL; + + // Expand the given topic name. char * remapped_topic_name = NULL; - ret = rcl_expand_topic_name( + rcl_ret_t ret = rcl_resolve_topic_name( + &(node_options->arguments), + global_args, topic_name, rcl_node_get_name(node), rcl_node_get_namespace(node), - &substitutions_map, *allocator, - &expanded_topic_name); - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } + false, + &remapped_topic_name); if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_TOPIC_NAME_INVALID; } else { - ret = RCL_RET_ERROR; + if (ret != RCL_RET_BAD_ALLOC) { + ret = RCL_RET_ERROR; + } } goto cleanup; } - RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Expanded topic name '%s'", expanded_topic_name); - - const rcl_node_options_t * node_options = rcl_node_get_options(node); - if (NULL == node_options) { - ret = RCL_RET_ERROR; - goto cleanup; - } - rcl_arguments_t * global_args = NULL; - if (node_options->use_global_arguments) { - global_args = &(node->context->global_arguments); - } - ret = rcl_remap_topic_name( - &(node_options->arguments), global_args, expanded_topic_name, - rcl_node_get_name(node), rcl_node_get_namespace(node), *allocator, &remapped_topic_name); - if (RCL_RET_OK != ret) { - goto fail; - } else if (NULL == remapped_topic_name) { - remapped_topic_name = expanded_topic_name; - expanded_topic_name = NULL; - } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Expanded and remapped topic name '%s'", remapped_topic_name); // Validate the expanded topic name. int validation_result; @@ -208,12 +171,7 @@ rcl_subscription_init( ret = fail_ret; // Fall through to cleanup cleanup: - if (NULL != expanded_topic_name) { - allocator->deallocate(expanded_topic_name, allocator->state); - } - if (NULL != remapped_topic_name) { - allocator->deallocate(remapped_topic_name, allocator->state); - } + allocator->deallocate(remapped_topic_name, allocator->state); return ret; } From 1fa0952377a8735fb36bd5adf1b3f57ac52c9193 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 13:16:25 -0300 Subject: [PATCH 03/17] Even more convenient Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/resolve_topic_name.h | 45 ++++++++++++++++++++++++++++ rcl/src/rcl/resolve_topic_name.c | 28 +++++++++++++++++ rcl/src/rcl/subscription.c | 16 ++-------- 3 files changed, 75 insertions(+), 14 deletions(-) diff --git a/rcl/include/rcl/resolve_topic_name.h b/rcl/include/rcl/resolve_topic_name.h index b1ea08394..cd8264642 100644 --- a/rcl/include/rcl/resolve_topic_name.h +++ b/rcl/include/rcl/resolve_topic_name.h @@ -25,6 +25,8 @@ extern "C" #include "rcl/allocator.h" #include "rcl/arguments.h" #include "rcl/macros.h" +#include "rcl/node.h" +#include "rcl/node_options.h" #include "rcl/types.h" #include "rcl/visibility_control.h" @@ -87,6 +89,49 @@ rcl_resolve_topic_name( bool only_expand, char ** output_topic_name); +/// Expand a given topic name into a fully-qualified topic name and apply remapping rules. +/** + * See \ref rcl_resolve_topic_name_with_node() for more info. + * This overload takes a node pointer instead of + * `local_args`, `global_args`, `node_name` and `node_namespace`. + * + *
+ * Attribute | Adherence + * ------------------ | ------------- + * Allocates Memory | Yes + * Thread-Safe | Yes + * Uses Atomics | No + * Lock-Free | Yes + * + * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. + * \param[in] input_topic_name Topic name to be expanded and remapped. + * \param[in] allocator The allocator to be used when creating the output topic. + * \param[in] only_expand if `true`, remmapping rules aren't applied. + * \param[out] output_topic_name Output char * pointer. + * \return `RCL_RET_OK` if the topic name was expanded successfully, or + * \return `RCL_RET_INVALID_ARGUMENT` if any of input_topic_name, node_name, node_namespace + * or output_topic_name are NULL, or + * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or + * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or + * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid + * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid + * (see \ref rmw_validate_node_name()), or + * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid + * (see \ref rmw_validate_namespace()), or + * \return `RCL_RET_UNKNOWN_SUBSTITUTION` for unknown substitutions in name, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_resolve_topic_name_with_node( + const rcl_node_t * node, + const char * input_topic_name, + rcl_allocator_t allocator, + bool only_expand, + char ** output_topic_name); + #ifdef __cplusplus } #endif diff --git a/rcl/src/rcl/resolve_topic_name.c b/rcl/src/rcl/resolve_topic_name.c index 6377fddd6..969712bac 100644 --- a/rcl/src/rcl/resolve_topic_name.c +++ b/rcl/src/rcl/resolve_topic_name.c @@ -114,3 +114,31 @@ rcl_resolve_topic_name( allocator.deallocate(expanded_topic_name, allocator.state); return ret; } + +rcl_ret_t +rcl_resolve_topic_name_with_node( + const rcl_node_t * node, + const char * input_topic_name, + rcl_allocator_t allocator, + bool only_expand, + char ** output_topic_name) +{ + const rcl_node_options_t * node_options = rcl_node_get_options(node); + if (NULL == node_options) { + return RCL_RET_ERROR; + } + rcl_arguments_t * global_args = NULL; + if (node_options->use_global_arguments) { + global_args = &(node->context->global_arguments); + } + + return rcl_resolve_topic_name( + &(node_options->arguments), + global_args, + input_topic_name, + rcl_node_get_name(node), + rcl_node_get_namespace(node), + allocator, + only_expand, + output_topic_name); +} diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index e5cd9f989..328aecbf6 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -67,23 +67,11 @@ rcl_subscription_init( return RCL_RET_ALREADY_INIT; } - const rcl_node_options_t * node_options = rcl_node_get_options(node); - if (NULL == node_options) { - return RCL_RET_ERROR; - } - rcl_arguments_t * global_args = NULL; - if (node_options->use_global_arguments) { - global_args = &(node->context->global_arguments); - } - // Expand the given topic name. char * remapped_topic_name = NULL; - rcl_ret_t ret = rcl_resolve_topic_name( - &(node_options->arguments), - global_args, + rcl_ret_t ret = rcl_resolve_topic_name_with_node( + node, topic_name, - rcl_node_get_name(node), - rcl_node_get_namespace(node), *allocator, false, &remapped_topic_name); From f61e1dda7865cefb5eaa9d8288df09a1c5ff9ef2 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 13:23:26 -0300 Subject: [PATCH 04/17] Use it in publisher init Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/publisher.c | 65 +++++++------------------------------- rcl/src/rcl/subscription.c | 6 ++-- 2 files changed, 14 insertions(+), 57 deletions(-) diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 6662aca0f..f9bc10a7a 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -26,6 +26,7 @@ extern "C" #include "rcl/error_handling.h" #include "rcl/expand_topic_name.h" #include "rcl/remap.h" +#include "rcl/resolve_topic_name.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rmw/error_handling.h" @@ -105,63 +106,26 @@ rcl_publisher_init( } return RCL_RET_ERROR; } - char * expanded_topic_name = NULL; + char * remapped_topic_name = NULL; - ret = rcl_expand_topic_name( + ret = rcl_resolve_topic_name_with_node( + node, topic_name, - rcl_node_get_name(node), - rcl_node_get_namespace(node), - &substitutions_map, *allocator, - &expanded_topic_name); - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } + false, + &remapped_topic_name); + if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_TOPIC_NAME_INVALID; - } else { + } else if (ret != RCL_RET_BAD_ALLOC) { ret = RCL_RET_ERROR; } goto cleanup; } - RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Expanded topic name '%s'", expanded_topic_name); - - const rcl_node_options_t * node_options = rcl_node_get_options(node); - if (NULL == node_options) { - ret = RCL_RET_ERROR; - goto cleanup; - } - rcl_arguments_t * global_args = NULL; - if (node_options->use_global_arguments) { - global_args = &(node->context->global_arguments); - } - ret = rcl_remap_topic_name( - &(node_options->arguments), global_args, expanded_topic_name, - rcl_node_get_name(node), rcl_node_get_namespace(node), *allocator, &remapped_topic_name); - if (RCL_RET_OK != ret) { - goto fail; - } else if (NULL == remapped_topic_name) { - remapped_topic_name = expanded_topic_name; - expanded_topic_name = NULL; - } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Expanded and remapped topic name '%s'", remapped_topic_name); - // Validate the expanded topic name. - int validation_result; - rmw_ret_t rmw_ret = rmw_validate_full_topic_name(remapped_topic_name, &validation_result, NULL); - if (rmw_ret != RMW_RET_OK) { - RCL_SET_ERROR_MSG(rmw_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } - if (validation_result != RMW_TOPIC_VALID) { - RCL_SET_ERROR_MSG(rmw_full_topic_name_validation_result_string(validation_result)); - ret = RCL_RET_TOPIC_NAME_INVALID; - goto cleanup; - } // Allocate space for the implementation struct. publisher->impl = (rcl_publisher_impl_t *)allocator->allocate( sizeof(rcl_publisher_impl_t), allocator->state); @@ -180,7 +144,7 @@ rcl_publisher_init( RCL_CHECK_FOR_NULL_WITH_MSG( publisher->impl->rmw_handle, rmw_get_error_string().str, goto fail); // get actual qos, and store it - rmw_ret = rmw_publisher_get_actual_qos( + rmw_ret_t rmw_ret = rmw_publisher_get_actual_qos( publisher->impl->rmw_handle, &publisher->impl->actual_qos); if (RMW_RET_OK != rmw_ret) { @@ -221,12 +185,7 @@ rcl_publisher_init( ret = fail_ret; // Fall through to cleanup cleanup: - if (NULL != expanded_topic_name) { - allocator->deallocate(expanded_topic_name, allocator->state); - } - if (NULL != remapped_topic_name) { - allocator->deallocate(remapped_topic_name, allocator->state); - } + allocator->deallocate(remapped_topic_name, allocator->state); return ret; } diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 328aecbf6..8b962277d 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -78,10 +78,8 @@ rcl_subscription_init( if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_TOPIC_NAME_INVALID; - } else { - if (ret != RCL_RET_BAD_ALLOC) { - ret = RCL_RET_ERROR; - } + } else if (ret != RCL_RET_BAD_ALLOC) { + ret = RCL_RET_ERROR; } goto cleanup; } From 15f6c34aef85251bfbd05ffced7cd1c75d406702 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 14:12:22 -0300 Subject: [PATCH 05/17] Adapt to services and clients Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 2 +- .../{resolve_topic_name.h => resolve_name.h} | 62 ++++++++------ rcl/src/rcl/client.c | 82 ++---------------- rcl/src/rcl/publisher.c | 7 +- rcl/src/rcl/remap.c | 17 ++-- rcl/src/rcl/remap_impl.h | 13 +++ .../{resolve_topic_name.c => resolve_name.c} | 35 ++++---- rcl/src/rcl/service.c | 85 +++---------------- rcl/src/rcl/subscription.c | 17 +--- 9 files changed, 99 insertions(+), 221 deletions(-) rename rcl/include/rcl/{resolve_topic_name.h => resolve_name.h} (73%) rename rcl/src/rcl/{resolve_topic_name.c => resolve_name.c} (86%) diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index 75662b800..9d46841fa 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -53,7 +53,7 @@ set(${PROJECT_NAME}_sources src/rcl/node_options.c src/rcl/publisher.c src/rcl/remap.c - src/rcl/resolve_topic_name.c + src/rcl/resolve_name.c src/rcl/rmw_implementation_identifier_check.c src/rcl/security.c src/rcl/service.c diff --git a/rcl/include/rcl/resolve_topic_name.h b/rcl/include/rcl/resolve_name.h similarity index 73% rename from rcl/include/rcl/resolve_topic_name.h rename to rcl/include/rcl/resolve_name.h index cd8264642..b85ae97d6 100644 --- a/rcl/include/rcl/resolve_topic_name.h +++ b/rcl/include/rcl/resolve_name.h @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef RCL__RESOLVE_TOPIC_NAME_H_ -#define RCL__RESOLVE_TOPIC_NAME_H_ +#ifndef RCL__RESOLVE_NAME_H_ +#define RCL__RESOLVE_NAME_H_ #ifdef __cplusplus extern "C" @@ -32,17 +32,17 @@ extern "C" /// Expand a given topic name into a fully-qualified topic name and apply remapping rules. /** - * \pre The input_topic_name, node_name, and node_namespace arguments must all be + * \pre The input_name, node_name, and node_namespace arguments must all be * valid, null terminated c strings. - * \post The output_topic_name will not be assigned a value in the event of an error. - * \post If assigned, the output_topic_name will be null terminated. + * \post The output_name will not be assigned a value in the event of an error. + * \post If assigned, the output_name will be null terminated. * Its memory must be freed by the user. * * This function uses the substitutions provided by rcl_get_default_topic_name_substitutions(), * and the additional ones explained in \ref expand_topic_name(). * * The remapping rules are provided by `local_args` and `global_args`, - * see \ref rcl_remap_topic_name(). + * see \ref rcl_remap_topic_name(), \ref rcl_remap_service_name(). * *
* Attribute | Adherence @@ -56,19 +56,25 @@ extern "C" * if NULL or zero-initialized then only global arguments are used. * \param[in] global_arguments Command line arguments to use if no local rules matched, or * `NULL` or zero-initialized to ignore global arguments. - * \param[in] input_topic_name Topic name to be expanded and remapped. - * \param[in] node_name Name of the node associated with the topic. - * \param[in] node_namespace Namespace of the node associated with the topic. - * \param[in] allocator The allocator to be used when creating the output topic. + * \param[in] input_name Topic or service name to be expanded and remapped. + * \param[in] node_name Name of the node associated with the topic/service. + * \param[in] node_namespace Namespace of the node associated with the topic/service. + * \param[in] allocator The allocator to be used when creating the output name. * \param[in] only_expand if `true`, remmapping rules aren't applied. - * \param[out] output_topic_name Output char * pointer. - * \return `RCL_RET_OK` if the topic name was expanded successfully, or - * \return `RCL_RET_INVALID_ARGUMENT` if any of input_topic_name, node_name, node_namespace - * or output_topic_name are NULL, or + * \param[out] output_name Output char * pointer. + * \return `RCL_RET_OK` if the name was expanded successfully, or + * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace + * or output_name are NULL, or * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_TOPIC_NAME_INVALID` if the resulting topic name is invalid + * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_SERVICE_NAME_INVALID` if the given service name is invalid + * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_SERVICE_NAME_INVALID` if the resulting service name is invalid + * (see \ref rcl_validate_topic_name()), or * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid * (see \ref rmw_validate_node_name()), or * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid @@ -79,19 +85,19 @@ extern "C" RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_resolve_topic_name( +rcl_resolve_name( const rcl_arguments_t * local_args, const rcl_arguments_t * global_args, - const char * input_topic_name, + const char * input_name, const char * node_name, const char * node_namespace, rcl_allocator_t allocator, - bool only_expand, - char ** output_topic_name); + bool is_service, + char ** output_name); /// Expand a given topic name into a fully-qualified topic name and apply remapping rules. /** - * See \ref rcl_resolve_topic_name_with_node() for more info. + * See \ref rcl_resolve_name_with_node() for more info. * This overload takes a node pointer instead of * `local_args`, `global_args`, `node_name` and `node_namespace`. * @@ -104,13 +110,13 @@ rcl_resolve_topic_name( * Lock-Free | Yes * * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. - * \param[in] input_topic_name Topic name to be expanded and remapped. + * \param[in] input_name Topic name to be expanded and remapped. * \param[in] allocator The allocator to be used when creating the output topic. * \param[in] only_expand if `true`, remmapping rules aren't applied. - * \param[out] output_topic_name Output char * pointer. + * \param[out] output_name Output char * pointer. * \return `RCL_RET_OK` if the topic name was expanded successfully, or - * \return `RCL_RET_INVALID_ARGUMENT` if any of input_topic_name, node_name, node_namespace - * or output_topic_name are NULL, or + * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace + * or output_name are NULL, or * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid @@ -125,15 +131,15 @@ rcl_resolve_topic_name( RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_resolve_topic_name_with_node( +rcl_resolve_name_with_node( const rcl_node_t * node, - const char * input_topic_name, + const char * input_name, rcl_allocator_t allocator, - bool only_expand, - char ** output_topic_name); + bool is_service, + char ** output_name); #ifdef __cplusplus } #endif -#endif // RCL__RESOLVE_TOPIC_NAME_H_ +#endif // RCL__RESOLVE_NAME_H_ diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 875236390..0a0538914 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -23,14 +23,12 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/expand_topic_name.h" -#include "rcl/remap.h" +#include "rcl/resolve_name.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rcutils/stdatomic_helper.h" #include "rmw/error_handling.h" #include "rmw/rmw.h" -#include "rmw/validate_full_topic_name.h" #include "tracetools/tracetools.h" #include "./common.h" @@ -76,46 +74,14 @@ rcl_client_init( return RCL_RET_ALREADY_INIT; } // Expand the given service name. - rcutils_allocator_t rcutils_allocator = *allocator; // implicit conversion to rcutils version - rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); - rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, rcutils_allocator); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - if (rcutils_ret == RCUTILS_RET_BAD_ALLOC) { - return RCL_RET_BAD_ALLOC; - } - return RCL_RET_ERROR; - } - rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); - if (ret != RCL_RET_OK) { - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, - "failed to fini string_map (%d) during error handling: %s\n", - rcutils_ret, - rcutils_get_error_string().str); - } - if (ret == RCL_RET_BAD_ALLOC) { - return ret; - } - return RCL_RET_ERROR; - } - char * expanded_service_name = NULL; char * remapped_service_name = NULL; - ret = rcl_expand_topic_name( + rcl_ret_t ret = rcl_resolve_name_with_node( + node, service_name, - rcl_node_get_name(node), - rcl_node_get_namespace(node), - &substitutions_map, *allocator, - &expanded_service_name); - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - allocator->deallocate(expanded_service_name, allocator->state); - return RCL_RET_ERROR; - } + true, + &remapped_service_name); + if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_SERVICE_NAME_INVALID; @@ -124,40 +90,8 @@ rcl_client_init( } goto cleanup; } - RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Expanded service name '%s'", expanded_service_name); - - const rcl_node_options_t * node_options = rcl_node_get_options(node); - if (NULL == node_options) { - ret = RCL_RET_ERROR; - goto cleanup; - } - rcl_arguments_t * global_args = NULL; - if (node_options->use_global_arguments) { - global_args = &(node->context->global_arguments); - } - ret = rcl_remap_service_name( - &(node_options->arguments), global_args, expanded_service_name, - rcl_node_get_name(node), rcl_node_get_namespace(node), *allocator, &remapped_service_name); - if (RCL_RET_OK != ret) { - goto fail; - } else if (NULL == remapped_service_name) { - remapped_service_name = expanded_service_name; - expanded_service_name = NULL; - } - - // Validate the expanded service name. - int validation_result; - rmw_ret_t rmw_ret = rmw_validate_full_topic_name(remapped_service_name, &validation_result, NULL); - if (rmw_ret != RMW_RET_OK) { - RCL_SET_ERROR_MSG(rmw_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } - if (validation_result != RMW_TOPIC_VALID) { - RCL_SET_ERROR_MSG(rmw_full_topic_name_validation_result_string(validation_result)); - ret = RCL_RET_SERVICE_NAME_INVALID; - goto cleanup; - } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Expanded and remapped service name '%s'", remapped_service_name); // Allocate space for the implementation struct. client->impl = (rcl_client_impl_t *)allocator->allocate( sizeof(rcl_client_impl_t), allocator->state); diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index f9bc10a7a..55df5b0ad 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -24,13 +24,10 @@ extern "C" #include "rcl/allocator.h" #include "rcl/error_handling.h" -#include "rcl/expand_topic_name.h" -#include "rcl/remap.h" -#include "rcl/resolve_topic_name.h" +#include "rcl/resolve_name.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rmw/error_handling.h" -#include "rmw/validate_full_topic_name.h" #include "tracetools/tracetools.h" #include "./common.h" @@ -108,7 +105,7 @@ rcl_publisher_init( } char * remapped_topic_name = NULL; - ret = rcl_resolve_topic_name_with_node( + ret = rcl_resolve_name_with_node( node, topic_name, *allocator, diff --git a/rcl/src/rcl/remap.c b/rcl/src/rcl/remap.c index d8af63f87..b982d1747 100644 --- a/rcl/src/rcl/remap.c +++ b/rcl/src/rcl/remap.c @@ -100,8 +100,9 @@ rcl_remap_copy( /// Get the first matching rule in a chain. /// \return RCL_RET_OK if no errors occurred while searching for a rule RCL_LOCAL +static rcl_ret_t -_rcl_remap_first_match( +rcl_remap_first_match( rcl_remap_t * remap_rules, int num_rules, rcl_remap_type_t type_bitmask, @@ -159,7 +160,7 @@ _rcl_remap_first_match( /// Remap from one name to another using rules matching a given type bitmask. RCL_LOCAL rcl_ret_t -_rcl_remap_name( +rcl_remap_name( const rcl_arguments_t * local_arguments, const rcl_arguments_t * global_arguments, rcl_remap_type_t type_bitmask, @@ -188,7 +189,7 @@ _rcl_remap_name( // Look at local rules first if (NULL != local_arguments) { - rcl_ret_t ret = _rcl_remap_first_match( + rcl_ret_t ret = rcl_remap_first_match( local_arguments->impl->remap_rules, local_arguments->impl->num_remap_rules, type_bitmask, name, node_name, node_namespace, substitutions, allocator, &rule); if (ret != RCL_RET_OK) { @@ -197,7 +198,7 @@ _rcl_remap_name( } // Check global rules if no local rule matched if (NULL == rule && NULL != global_arguments) { - rcl_ret_t ret = _rcl_remap_first_match( + rcl_ret_t ret = rcl_remap_first_match( global_arguments->impl->remap_rules, global_arguments->impl->num_remap_rules, type_bitmask, name, node_name, node_namespace, substitutions, allocator, &rule); if (ret != RCL_RET_OK) { @@ -244,7 +245,7 @@ rcl_remap_topic_name( if (RCUTILS_RET_OK == rcutils_ret) { ret = rcl_get_default_topic_name_substitutions(&substitutions); if (RCL_RET_OK == ret) { - ret = _rcl_remap_name( + ret = rcl_remap_name( local_arguments, global_arguments, RCL_TOPIC_REMAP, topic_name, node_name, node_namespace, &substitutions, allocator, output_name); } @@ -274,7 +275,7 @@ rcl_remap_service_name( if (rcutils_ret == RCUTILS_RET_OK) { ret = rcl_get_default_topic_name_substitutions(&substitutions); if (ret == RCL_RET_OK) { - ret = _rcl_remap_name( + ret = rcl_remap_name( local_arguments, global_arguments, RCL_SERVICE_REMAP, service_name, node_name, node_namespace, &substitutions, allocator, output_name); } @@ -299,7 +300,7 @@ rcl_remap_node_name( RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF(RCL_RET_ERROR); RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); - return _rcl_remap_name( + return rcl_remap_name( local_arguments, global_arguments, RCL_NODENAME_REMAP, NULL, node_name, NULL, NULL, allocator, output_name); } @@ -318,7 +319,7 @@ rcl_remap_node_namespace( RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF(RCL_RET_ERROR); RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); - return _rcl_remap_name( + return rcl_remap_name( local_arguments, global_arguments, RCL_NAMESPACE_REMAP, NULL, node_name, NULL, NULL, allocator, output_namespace); } diff --git a/rcl/src/rcl/remap_impl.h b/rcl/src/rcl/remap_impl.h index 898e58cb7..770672592 100644 --- a/rcl/src/rcl/remap_impl.h +++ b/rcl/src/rcl/remap_impl.h @@ -51,6 +51,19 @@ typedef struct rcl_remap_impl_t rcl_allocator_t allocator; } rcl_remap_impl_t; +RCL_LOCAL +rcl_ret_t +rcl_remap_name( + const rcl_arguments_t * local_arguments, + const rcl_arguments_t * global_arguments, + rcl_remap_type_t type_bitmask, + const char * name, + const char * node_name, + const char * node_namespace, + const rcutils_string_map_t * substitutions, + rcl_allocator_t allocator, + char ** output_name); + #ifdef __cplusplus } #endif diff --git a/rcl/src/rcl/resolve_topic_name.c b/rcl/src/rcl/resolve_name.c similarity index 86% rename from rcl/src/rcl/resolve_topic_name.c rename to rcl/src/rcl/resolve_name.c index 969712bac..f80909b27 100644 --- a/rcl/src/rcl/resolve_topic_name.c +++ b/rcl/src/rcl/resolve_name.c @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "rcl/resolve_topic_name.h" +#include "rcl/resolve_name.h" #include "rcutils/error_handling.h" #include "rcutils/logging_macros.h" @@ -25,15 +25,17 @@ #include "rcl/expand_topic_name.h" #include "rcl/remap.h" +#include "./remap_impl.h" + rcl_ret_t -rcl_resolve_topic_name( +rcl_resolve_name( const rcl_arguments_t * local_args, const rcl_arguments_t * global_args, const char * input_topic_name, const char * node_name, const char * node_namespace, rcl_allocator_t allocator, - bool only_expand, + bool is_service, char ** output_topic_name) { // the other arguments are checked by rcl_expand_topic_name() and rcl_remap_topic_name() @@ -70,14 +72,13 @@ rcl_resolve_topic_name( if (RCL_RET_OK != ret) { goto cleanup; } - if (!only_expand) { - // remap topic name - ret = rcl_remap_topic_name( - local_args, global_args, expanded_topic_name, - node_name, node_namespace, allocator, &remapped_topic_name); - if (RCL_RET_OK != ret) { - goto cleanup; - } + // remap topic name + ret = rcl_remap_name( + local_args, global_args, is_service ? RCL_SERVICE_REMAP : RCL_TOPIC_REMAP, + expanded_topic_name, node_name, node_namespace, &substitutions_map, allocator, + &remapped_topic_name); + if (RCL_RET_OK != ret) { + goto cleanup; } if (NULL == remapped_topic_name) { remapped_topic_name = expanded_topic_name; @@ -112,17 +113,21 @@ rcl_resolve_topic_name( rcutils_reset_error(); } allocator.deallocate(expanded_topic_name, allocator.state); + if (is_service && RCL_RET_TOPIC_NAME_INVALID == ret) { + ret = RCL_RET_SERVICE_NAME_INVALID; + } return ret; } rcl_ret_t -rcl_resolve_topic_name_with_node( +rcl_resolve_name_with_node( const rcl_node_t * node, const char * input_topic_name, rcl_allocator_t allocator, - bool only_expand, + bool is_service, char ** output_topic_name) { + // node get options checks `node` validity const rcl_node_options_t * node_options = rcl_node_get_options(node); if (NULL == node_options) { return RCL_RET_ERROR; @@ -132,13 +137,13 @@ rcl_resolve_topic_name_with_node( global_args = &(node->context->global_arguments); } - return rcl_resolve_topic_name( + return rcl_resolve_name( &(node_options->arguments), global_args, input_topic_name, rcl_node_get_name(node), rcl_node_get_namespace(node), allocator, - only_expand, + is_service, output_topic_name); } diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 196e4aab3..63aefbf0a 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -23,13 +23,11 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/expand_topic_name.h" -#include "rcl/remap.h" +#include "rcl/resolve_name.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rmw/error_handling.h" #include "rmw/rmw.h" -#include "rmw/validate_full_topic_name.h" #include "tracetools/tracetools.h" typedef struct rcl_service_impl_t @@ -80,88 +78,25 @@ rcl_service_init( return RCL_RET_ALREADY_INIT; } // Expand the given service name. - rcutils_allocator_t rcutils_allocator = *allocator; // implicit conversion to rcutils version - rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); - rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, rcutils_allocator); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - if (RCUTILS_RET_BAD_ALLOC == rcutils_ret) { - return RCL_RET_BAD_ALLOC; - } - return RCL_RET_ERROR; - } - rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); - if (ret != RCL_RET_OK) { - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, - "failed to fini string_map (%d) during error handling: %s", - rcutils_ret, - rcutils_get_error_string().str); - } - if (RCL_RET_BAD_ALLOC == ret) { - return ret; - } - return RCL_RET_ERROR; - } - char * expanded_service_name = NULL; char * remapped_service_name = NULL; - ret = rcl_expand_topic_name( + rcl_ret_t ret = rcl_resolve_name_with_node( + node, service_name, - rcl_node_get_name(node), - rcl_node_get_namespace(node), - &substitutions_map, *allocator, - &expanded_service_name); - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } + true, + &remapped_service_name); + if (ret != RCL_RET_OK) { - if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { + if (ret == RCL_RET_SERVICE_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_SERVICE_NAME_INVALID; - } else { + } else if (ret != RCL_RET_BAD_ALLOC) { ret = RCL_RET_ERROR; } goto cleanup; } - RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Expanded service name '%s'", expanded_service_name); - - const rcl_node_options_t * node_options = rcl_node_get_options(node); - if (NULL == node_options) { - ret = RCL_RET_ERROR; - goto cleanup; - } - rcl_arguments_t * global_args = NULL; - if (node_options->use_global_arguments) { - global_args = &(node->context->global_arguments); - } - ret = rcl_remap_service_name( - &(node_options->arguments), global_args, expanded_service_name, - rcl_node_get_name(node), rcl_node_get_namespace(node), *allocator, &remapped_service_name); - if (RCL_RET_OK != ret) { - goto fail; - } else if (NULL == remapped_service_name) { - remapped_service_name = expanded_service_name; - expanded_service_name = NULL; - } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Expanded and remapped service name '%s'", remapped_service_name); - // Validate the expanded service name. - int validation_result; - rmw_ret_t rmw_ret = rmw_validate_full_topic_name(remapped_service_name, &validation_result, NULL); - if (rmw_ret != RMW_RET_OK) { - RCL_SET_ERROR_MSG(rmw_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } - if (validation_result != RMW_TOPIC_VALID) { - RCL_SET_ERROR_MSG(rmw_full_topic_name_validation_result_string(validation_result)); - ret = RCL_RET_SERVICE_NAME_INVALID; - goto cleanup; - } // Allocate space for the implementation struct. service->impl = (rcl_service_impl_t *)allocator->allocate( sizeof(rcl_service_impl_t), allocator->state); diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 8b962277d..8b71fd256 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -22,7 +22,7 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/resolve_topic_name.h" +#include "rcl/resolve_name.h" #include "rcutils/logging_macros.h" #include "rmw/error_handling.h" #include "rmw/validate_full_topic_name.h" @@ -69,7 +69,7 @@ rcl_subscription_init( // Expand the given topic name. char * remapped_topic_name = NULL; - rcl_ret_t ret = rcl_resolve_topic_name_with_node( + rcl_ret_t ret = rcl_resolve_name_with_node( node, topic_name, *allocator, @@ -86,19 +86,6 @@ rcl_subscription_init( RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Expanded and remapped topic name '%s'", remapped_topic_name); - // Validate the expanded topic name. - int validation_result; - rmw_ret_t rmw_ret = rmw_validate_full_topic_name(remapped_topic_name, &validation_result, NULL); - if (rmw_ret != RMW_RET_OK) { - RCL_SET_ERROR_MSG(rmw_get_error_string().str); - ret = RCL_RET_ERROR; - goto cleanup; - } - if (validation_result != RMW_TOPIC_VALID) { - RCL_SET_ERROR_MSG(rmw_full_topic_name_validation_result_string(validation_result)); - ret = RCL_RET_TOPIC_NAME_INVALID; - goto cleanup; - } // Allocate memory for the implementation struct. subscription->impl = (rcl_subscription_impl_t *)allocator->allocate( sizeof(rcl_subscription_impl_t), allocator->state); From 09303465bcccda9a1485c0ae33734208ae3214b1 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 14:26:34 -0300 Subject: [PATCH 06/17] bugs Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/resolve_name.h | 4 ++-- rcl/src/rcl/resolve_name.c | 20 +++++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/rcl/include/rcl/resolve_name.h b/rcl/include/rcl/resolve_name.h index b85ae97d6..fb261c19a 100644 --- a/rcl/include/rcl/resolve_name.h +++ b/rcl/include/rcl/resolve_name.h @@ -60,7 +60,7 @@ extern "C" * \param[in] node_name Name of the node associated with the topic/service. * \param[in] node_namespace Namespace of the node associated with the topic/service. * \param[in] allocator The allocator to be used when creating the output name. - * \param[in] only_expand if `true`, remmapping rules aren't applied. + * \param[in] is_service for services use `true`, for topics use `false`. * \param[out] output_name Output char * pointer. * \return `RCL_RET_OK` if the name was expanded successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace @@ -112,7 +112,7 @@ rcl_resolve_name( * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. * \param[in] input_name Topic name to be expanded and remapped. * \param[in] allocator The allocator to be used when creating the output topic. - * \param[in] only_expand if `true`, remmapping rules aren't applied. + * \param[in] is_service for services use `true`, for topics use `false`. * \param[out] output_name Output char * pointer. * \return `RCL_RET_OK` if the topic name was expanded successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace diff --git a/rcl/src/rcl/resolve_name.c b/rcl/src/rcl/resolve_name.c index f80909b27..7a60cd976 100644 --- a/rcl/src/rcl/resolve_name.c +++ b/rcl/src/rcl/resolve_name.c @@ -44,9 +44,9 @@ rcl_resolve_name( rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, allocator); if (rcutils_ret != RCUTILS_RET_OK) { - const char * error = rcutils_get_error_string().str; + rcutils_error_string_t error = rcutils_get_error_string(); rcutils_reset_error(); - RCL_SET_ERROR_MSG(error); + RCL_SET_ERROR_MSG(error.str); if (RCUTILS_RET_BAD_ALLOC == rcutils_ret) { return RCL_RET_BAD_ALLOC; } @@ -105,12 +105,18 @@ rcl_resolve_name( cleanup: rcutils_ret = rcutils_string_map_fini(&substitutions_map); if (rcutils_ret != RCUTILS_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, - "failed to fini string_map (%d) during error handling: %s", - rcutils_ret, - rcutils_get_error_string().str); + rcutils_error_string_t error = rcutils_get_error_string(); rcutils_reset_error(); + if (RCL_RET_OK != ret) { + RCL_SET_ERROR_MSG(error); + ret = RCL_RET_ERROR; + } else { + RCUTILS_LOG_ERROR_NAMED( + ROS_PACKAGE_NAME, + "failed to fini string_map (%d) during error handling: %s", + rcutils_ret, + error.str); + } } allocator.deallocate(expanded_topic_name, allocator.state); if (is_service && RCL_RET_TOPIC_NAME_INVALID == ret) { From 48ff4634c11f7cd72fe71a16d7d9446d981485c1 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 14:30:52 -0300 Subject: [PATCH 07/17] More fixes Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/client.c | 7 +------ rcl/src/rcl/publisher.c | 28 +--------------------------- rcl/src/rcl/resolve_name.c | 2 +- rcl/src/rcl/service.c | 7 +------ rcl/src/rcl/subscription.c | 2 +- 5 files changed, 5 insertions(+), 41 deletions(-) diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 0a0538914..af048f6cc 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -129,12 +129,7 @@ rcl_client_init( ret = fail_ret; // Fall through to cleanup cleanup: - if (NULL != expanded_service_name) { - allocator->deallocate(expanded_service_name, allocator->state); - } - if (NULL != remapped_service_name) { - allocator->deallocate(remapped_service_name, allocator->state); - } + allocator->deallocate(remapped_service_name, allocator->state); return ret; } diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 55df5b0ad..83d8fce5a 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -78,34 +78,8 @@ rcl_publisher_init( // Expand the given topic name. - rcutils_allocator_t rcutils_allocator = *allocator; // implicit conversion to rcutils version - rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); - rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, rcutils_allocator); - if (rcutils_ret != RCUTILS_RET_OK) { - RCL_SET_ERROR_MSG(rcutils_get_error_string().str); - if (rcutils_ret == RCUTILS_RET_BAD_ALLOC) { - return RCL_RET_BAD_ALLOC; - } - return RCL_RET_ERROR; - } - rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); - if (ret != RCL_RET_OK) { - rcutils_ret = rcutils_string_map_fini(&substitutions_map); - if (rcutils_ret != RCUTILS_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, - "failed to fini string_map (%d) during error handling: %s", - rcutils_ret, - rcutils_get_error_string().str); - } - if (ret == RCL_RET_BAD_ALLOC) { - return ret; - } - return RCL_RET_ERROR; - } - char * remapped_topic_name = NULL; - ret = rcl_resolve_name_with_node( + rcl_ret_t ret = rcl_resolve_name_with_node( node, topic_name, *allocator, diff --git a/rcl/src/rcl/resolve_name.c b/rcl/src/rcl/resolve_name.c index 7a60cd976..b0322f4f4 100644 --- a/rcl/src/rcl/resolve_name.c +++ b/rcl/src/rcl/resolve_name.c @@ -108,7 +108,7 @@ rcl_resolve_name( rcutils_error_string_t error = rcutils_get_error_string(); rcutils_reset_error(); if (RCL_RET_OK != ret) { - RCL_SET_ERROR_MSG(error); + RCL_SET_ERROR_MSG(error.str); ret = RCL_RET_ERROR; } else { RCUTILS_LOG_ERROR_NAMED( diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 63aefbf0a..b2acfcde7 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -140,12 +140,7 @@ rcl_service_init( ret = fail_ret; // Fall through to clean up cleanup: - if (NULL != expanded_service_name) { - allocator->deallocate(expanded_service_name, allocator->state); - } - if (NULL != remapped_service_name) { - allocator->deallocate(remapped_service_name, allocator->state); - } + allocator->deallocate(remapped_service_name, allocator->state); return ret; } diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 8b71fd256..629cd4196 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -105,7 +105,7 @@ rcl_subscription_init( goto fail; } // get actual qos, and store it - rmw_ret = rmw_subscription_get_actual_qos( + rmw_ret_t rmw_ret = rmw_subscription_get_actual_qos( subscription->impl->rmw_handle, &subscription->impl->actual_qos); if (RMW_RET_OK != rmw_ret) { From 45731fae821e778e52e7eee568dc31cdc3a798b5 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 15:08:57 -0300 Subject: [PATCH 08/17] More test fixes Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/client.c | 3 ++- rcl/src/rcl/publisher.c | 4 +--- rcl/src/rcl/resolve_name.c | 5 +++-- rcl/src/rcl/service.c | 4 ++-- rcl/src/rcl/subscription.c | 2 +- rcl/test/rcl/test_publisher.cpp | 17 ----------------- rcl/test/rcl/test_subscription.cpp | 6 ++++++ 7 files changed, 15 insertions(+), 26 deletions(-) diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index af048f6cc..a8c7be403 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -73,6 +73,7 @@ rcl_client_init( RCL_SET_ERROR_MSG("client already initialized, or memory was unintialized"); return RCL_RET_ALREADY_INIT; } + // Expand the given service name. char * remapped_service_name = NULL; rcl_ret_t ret = rcl_resolve_name_with_node( @@ -81,7 +82,6 @@ rcl_client_init( *allocator, true, &remapped_service_name); - if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_SERVICE_NAME_INVALID; @@ -92,6 +92,7 @@ rcl_client_init( } RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Expanded and remapped service name '%s'", remapped_service_name); + // Allocate space for the implementation struct. client->impl = (rcl_client_impl_t *)allocator->allocate( sizeof(rcl_client_impl_t), allocator->state); diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 83d8fce5a..252767d1c 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -76,8 +76,7 @@ rcl_publisher_init( RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Initializing publisher for topic name '%s'", topic_name); - - // Expand the given topic name. + // Expand and remap the given topic name. char * remapped_topic_name = NULL; rcl_ret_t ret = rcl_resolve_name_with_node( node, @@ -85,7 +84,6 @@ rcl_publisher_init( *allocator, false, &remapped_topic_name); - if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_TOPIC_NAME_INVALID; diff --git a/rcl/src/rcl/resolve_name.c b/rcl/src/rcl/resolve_name.c index b0322f4f4..5e6960b32 100644 --- a/rcl/src/rcl/resolve_name.c +++ b/rcl/src/rcl/resolve_name.c @@ -100,14 +100,14 @@ rcl_resolve_name( goto cleanup; } *output_topic_name = remapped_topic_name; - return RCL_RET_OK; + remapped_topic_name = NULL; cleanup: rcutils_ret = rcutils_string_map_fini(&substitutions_map); if (rcutils_ret != RCUTILS_RET_OK) { rcutils_error_string_t error = rcutils_get_error_string(); rcutils_reset_error(); - if (RCL_RET_OK != ret) { + if (RCL_RET_OK == ret) { RCL_SET_ERROR_MSG(error.str); ret = RCL_RET_ERROR; } else { @@ -119,6 +119,7 @@ rcl_resolve_name( } } allocator.deallocate(expanded_topic_name, allocator.state); + allocator.deallocate(remapped_topic_name, allocator.state); if (is_service && RCL_RET_TOPIC_NAME_INVALID == ret) { ret = RCL_RET_SERVICE_NAME_INVALID; } diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index b2acfcde7..0ccb3d2ff 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -77,7 +77,8 @@ rcl_service_init( RCL_SET_ERROR_MSG("service already initialized, or memory was unintialized"); return RCL_RET_ALREADY_INIT; } - // Expand the given service name. + + // Expand and remap the given service name. char * remapped_service_name = NULL; rcl_ret_t ret = rcl_resolve_name_with_node( node, @@ -85,7 +86,6 @@ rcl_service_init( *allocator, true, &remapped_service_name); - if (ret != RCL_RET_OK) { if (ret == RCL_RET_SERVICE_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_SERVICE_NAME_INVALID; diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 629cd4196..96d10c72a 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -67,7 +67,7 @@ rcl_subscription_init( return RCL_RET_ALREADY_INIT; } - // Expand the given topic name. + // Expand and remap the given topic name. char * remapped_topic_name = NULL; rcl_ret_t ret = rcl_resolve_name_with_node( node, diff --git a/rcl/test/rcl/test_publisher.cpp b/rcl/test/rcl/test_publisher.cpp index 3fcbe0227..cf28011f7 100644 --- a/rcl/test/rcl/test_publisher.cpp +++ b/rcl/test/rcl/test_publisher.cpp @@ -788,23 +788,6 @@ TEST_F(CLASSNAME(TestPublisherFixture, RMW_IMPLEMENTATION), test_mocks_fail_publ EXPECT_EQ(RCL_RET_ERROR, ret) << rcl_get_error_string().str; rcl_reset_error(); } - { - // Internal failure when fini rcutils_string_map returns error, targets rcl_remap_topic_name - auto mock = mocking_utils::patch( - "lib:rcl", rcutils_string_map_init, [](auto...) { - static int counter = 1; - if (counter == 1) { - counter++; - return RCUTILS_RET_OK; - } else { - // This makes rcl_remap_topic_name fail - return RCUTILS_RET_ERROR; - } - }); - ret = rcl_publisher_init(&publisher, this->node_ptr, ts, topic_name, &publisher_options); - EXPECT_EQ(RCL_RET_ERROR, ret) << rcl_get_error_string().str; - rcl_reset_error(); - } { // Internal rmw failure validating topic name auto mock = mocking_utils::patch_and_return( diff --git a/rcl/test/rcl/test_subscription.cpp b/rcl/test/rcl/test_subscription.cpp index 2d0e4b635..54bfd2af8 100644 --- a/rcl/test/rcl/test_subscription.cpp +++ b/rcl/test/rcl/test_subscription.cpp @@ -204,9 +204,11 @@ TEST_F(CLASSNAME(TestSubscriptionFixture, RMW_IMPLEMENTATION), test_subscription { auto mock = mocking_utils::inject_on_return("lib:rcl", rcutils_string_map_fini, RCUTILS_RET_ERROR); + fprintf(stderr, "dubious 1\n"); ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_ERROR, ret); rcl_reset_error(); + fprintf(stderr, "end dubious 1\n"); } { rmw_ret_t rmw_validate_full_topic_name_returns = RMW_RET_OK; @@ -216,14 +218,18 @@ TEST_F(CLASSNAME(TestSubscriptionFixture, RMW_IMPLEMENTATION), test_subscription *result = RMW_TOPIC_INVALID_TOO_LONG; return rmw_validate_full_topic_name_returns; }); + fprintf(stderr, "dubious 2\n"); ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_TOPIC_NAME_INVALID, ret); rcl_reset_error(); + fprintf(stderr, "end dubious 2\n"); + fprintf(stderr, "dubious 3\n"); rmw_validate_full_topic_name_returns = RMW_RET_ERROR; ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_ERROR, ret); rcl_reset_error(); + fprintf(stderr, "end dubious 3\n"); } { auto mock = From d4fe4295e707e80f38aba08d7e440e4b69e7eb88 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 15:33:03 -0300 Subject: [PATCH 09/17] Another cleanup Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 2 +- rcl/include/rcl/node.h | 39 +++++ rcl/include/rcl/resolve_name.h | 145 ------------------ rcl/src/rcl/client.c | 4 +- .../{resolve_name.c => node_resolve_name.c} | 5 +- rcl/src/rcl/publisher.c | 4 +- rcl/src/rcl/remap.c | 1 - rcl/src/rcl/service.c | 4 +- rcl/src/rcl/subscription.c | 4 +- rcl/test/rcl/test_subscription.cpp | 6 - 10 files changed, 51 insertions(+), 163 deletions(-) delete mode 100644 rcl/include/rcl/resolve_name.h rename rcl/src/rcl/{resolve_name.c => node_resolve_name.c} (98%) diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index 9d46841fa..273c8315d 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -53,7 +53,7 @@ set(${PROJECT_NAME}_sources src/rcl/node_options.c src/rcl/publisher.c src/rcl/remap.c - src/rcl/resolve_name.c + src/rcl/node_resolve_name.c src/rcl/rmw_implementation_identifier_check.c src/rcl/security.c src/rcl/service.c diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index 900edb50e..f79e48f96 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -490,6 +490,45 @@ RCL_WARN_UNUSED const char * rcl_node_get_logger_name(const rcl_node_t * node); +/// Expand a given name into a fully-qualified topic name and apply remapping rules. +/** + *
+ * Attribute | Adherence + * ------------------ | ------------- + * Allocates Memory | Yes + * Thread-Safe | Yes + * Uses Atomics | No + * Lock-Free | Yes + * + * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. + * \param[in] input_name Topic name to be expanded and remapped. + * \param[in] allocator The allocator to be used when creating the output topic. + * \param[in] is_service for services use `true`, for topics use `false`. + * \param[out] output_name Output char * pointer. + * \return `RCL_RET_OK` if the topic name was expanded successfully, or + * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace + * or output_name are NULL, or + * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or + * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or + * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid + * (see \ref rcl_validate_topic_name()), or + * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid + * (see \ref rmw_validate_node_name()), or + * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid + * (see \ref rmw_validate_namespace()), or + * \return `RCL_RET_UNKNOWN_SUBSTITUTION` for unknown substitutions in name, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_node_resolve_name( + const rcl_node_t * node, + const char * input_name, + rcl_allocator_t allocator, + bool is_service, + char ** output_name); + #ifdef __cplusplus } #endif diff --git a/rcl/include/rcl/resolve_name.h b/rcl/include/rcl/resolve_name.h deleted file mode 100644 index fb261c19a..000000000 --- a/rcl/include/rcl/resolve_name.h +++ /dev/null @@ -1,145 +0,0 @@ -// Copyright 2020 Open Source Robotics Foundation, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef RCL__RESOLVE_NAME_H_ -#define RCL__RESOLVE_NAME_H_ - -#ifdef __cplusplus -extern "C" -{ -#endif - -#include - -#include "rcl/allocator.h" -#include "rcl/arguments.h" -#include "rcl/macros.h" -#include "rcl/node.h" -#include "rcl/node_options.h" -#include "rcl/types.h" -#include "rcl/visibility_control.h" - -/// Expand a given topic name into a fully-qualified topic name and apply remapping rules. -/** - * \pre The input_name, node_name, and node_namespace arguments must all be - * valid, null terminated c strings. - * \post The output_name will not be assigned a value in the event of an error. - * \post If assigned, the output_name will be null terminated. - * Its memory must be freed by the user. - * - * This function uses the substitutions provided by rcl_get_default_topic_name_substitutions(), - * and the additional ones explained in \ref expand_topic_name(). - * - * The remapping rules are provided by `local_args` and `global_args`, - * see \ref rcl_remap_topic_name(), \ref rcl_remap_service_name(). - * - *
- * Attribute | Adherence - * ------------------ | ------------- - * Allocates Memory | Yes - * Thread-Safe | Yes - * Uses Atomics | No - * Lock-Free | Yes - * - * \param[in] local_arguments Command line arguments to be used before global arguments, or - * if NULL or zero-initialized then only global arguments are used. - * \param[in] global_arguments Command line arguments to use if no local rules matched, or - * `NULL` or zero-initialized to ignore global arguments. - * \param[in] input_name Topic or service name to be expanded and remapped. - * \param[in] node_name Name of the node associated with the topic/service. - * \param[in] node_namespace Namespace of the node associated with the topic/service. - * \param[in] allocator The allocator to be used when creating the output name. - * \param[in] is_service for services use `true`, for topics use `false`. - * \param[out] output_name Output char * pointer. - * \return `RCL_RET_OK` if the name was expanded successfully, or - * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace - * or output_name are NULL, or - * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or - * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or - * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid - * (see \ref rcl_validate_topic_name()), or - * \return `RCL_RET_TOPIC_NAME_INVALID` if the resulting topic name is invalid - * (see \ref rcl_validate_topic_name()), or - * \return `RCL_RET_SERVICE_NAME_INVALID` if the given service name is invalid - * (see \ref rcl_validate_topic_name()), or - * \return `RCL_RET_SERVICE_NAME_INVALID` if the resulting service name is invalid - * (see \ref rcl_validate_topic_name()), or - * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid - * (see \ref rmw_validate_node_name()), or - * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid - * (see \ref rmw_validate_namespace()), or - * \return `RCL_RET_UNKNOWN_SUBSTITUTION` for unknown substitutions in name, or - * \return `RCL_RET_ERROR` if an unspecified error occurs. - */ -RCL_PUBLIC -RCL_WARN_UNUSED -rcl_ret_t -rcl_resolve_name( - const rcl_arguments_t * local_args, - const rcl_arguments_t * global_args, - const char * input_name, - const char * node_name, - const char * node_namespace, - rcl_allocator_t allocator, - bool is_service, - char ** output_name); - -/// Expand a given topic name into a fully-qualified topic name and apply remapping rules. -/** - * See \ref rcl_resolve_name_with_node() for more info. - * This overload takes a node pointer instead of - * `local_args`, `global_args`, `node_name` and `node_namespace`. - * - *
- * Attribute | Adherence - * ------------------ | ------------- - * Allocates Memory | Yes - * Thread-Safe | Yes - * Uses Atomics | No - * Lock-Free | Yes - * - * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. - * \param[in] input_name Topic name to be expanded and remapped. - * \param[in] allocator The allocator to be used when creating the output topic. - * \param[in] is_service for services use `true`, for topics use `false`. - * \param[out] output_name Output char * pointer. - * \return `RCL_RET_OK` if the topic name was expanded successfully, or - * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace - * or output_name are NULL, or - * \return `RCL_RET_INVALID_ARGUMENT` if both local_args and global_args are NULL, or - * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or - * \return `RCL_RET_TOPIC_NAME_INVALID` if the given topic name is invalid - * (see \ref rcl_validate_topic_name()), or - * \return `RCL_RET_NODE_INVALID_NAME` if the given node name is invalid - * (see \ref rmw_validate_node_name()), or - * \return `RCL_RET_NODE_INVALID_NAMESPACE` if the given node namespace is invalid - * (see \ref rmw_validate_namespace()), or - * \return `RCL_RET_UNKNOWN_SUBSTITUTION` for unknown substitutions in name, or - * \return `RCL_RET_ERROR` if an unspecified error occurs. - */ -RCL_PUBLIC -RCL_WARN_UNUSED -rcl_ret_t -rcl_resolve_name_with_node( - const rcl_node_t * node, - const char * input_name, - rcl_allocator_t allocator, - bool is_service, - char ** output_name); - -#ifdef __cplusplus -} -#endif - -#endif // RCL__RESOLVE_NAME_H_ diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index a8c7be403..d226186cf 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -23,7 +23,7 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/resolve_name.h" +#include "rcl/node.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rcutils/stdatomic_helper.h" @@ -76,7 +76,7 @@ rcl_client_init( // Expand the given service name. char * remapped_service_name = NULL; - rcl_ret_t ret = rcl_resolve_name_with_node( + rcl_ret_t ret = rcl_node_resolve_name( node, service_name, *allocator, diff --git a/rcl/src/rcl/resolve_name.c b/rcl/src/rcl/node_resolve_name.c similarity index 98% rename from rcl/src/rcl/resolve_name.c rename to rcl/src/rcl/node_resolve_name.c index 5e6960b32..1660e3886 100644 --- a/rcl/src/rcl/resolve_name.c +++ b/rcl/src/rcl/node_resolve_name.c @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "rcl/resolve_name.h" +#include "rcl/node.h" #include "rcutils/error_handling.h" #include "rcutils/logging_macros.h" @@ -27,6 +27,7 @@ #include "./remap_impl.h" +static rcl_ret_t rcl_resolve_name( const rcl_arguments_t * local_args, @@ -127,7 +128,7 @@ rcl_resolve_name( } rcl_ret_t -rcl_resolve_name_with_node( +rcl_node_resolve_name( const rcl_node_t * node, const char * input_topic_name, rcl_allocator_t allocator, diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 252767d1c..71291ae0f 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -24,7 +24,7 @@ extern "C" #include "rcl/allocator.h" #include "rcl/error_handling.h" -#include "rcl/resolve_name.h" +#include "rcl/node.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rmw/error_handling.h" @@ -78,7 +78,7 @@ rcl_publisher_init( // Expand and remap the given topic name. char * remapped_topic_name = NULL; - rcl_ret_t ret = rcl_resolve_name_with_node( + rcl_ret_t ret = rcl_node_resolve_name( node, topic_name, *allocator, diff --git a/rcl/src/rcl/remap.c b/rcl/src/rcl/remap.c index b982d1747..5db08623c 100644 --- a/rcl/src/rcl/remap.c +++ b/rcl/src/rcl/remap.c @@ -99,7 +99,6 @@ rcl_remap_copy( /// Get the first matching rule in a chain. /// \return RCL_RET_OK if no errors occurred while searching for a rule -RCL_LOCAL static rcl_ret_t rcl_remap_first_match( diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 0ccb3d2ff..e1c26f4a7 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -23,7 +23,7 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/resolve_name.h" +#include "rcl/node.h" #include "rcutils/logging_macros.h" #include "rcutils/macros.h" #include "rmw/error_handling.h" @@ -80,7 +80,7 @@ rcl_service_init( // Expand and remap the given service name. char * remapped_service_name = NULL; - rcl_ret_t ret = rcl_resolve_name_with_node( + rcl_ret_t ret = rcl_node_resolve_name( node, service_name, *allocator, diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 96d10c72a..60c79d550 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -22,7 +22,7 @@ extern "C" #include #include "rcl/error_handling.h" -#include "rcl/resolve_name.h" +#include "rcl/node.h" #include "rcutils/logging_macros.h" #include "rmw/error_handling.h" #include "rmw/validate_full_topic_name.h" @@ -69,7 +69,7 @@ rcl_subscription_init( // Expand and remap the given topic name. char * remapped_topic_name = NULL; - rcl_ret_t ret = rcl_resolve_name_with_node( + rcl_ret_t ret = rcl_node_resolve_name( node, topic_name, *allocator, diff --git a/rcl/test/rcl/test_subscription.cpp b/rcl/test/rcl/test_subscription.cpp index 54bfd2af8..2d0e4b635 100644 --- a/rcl/test/rcl/test_subscription.cpp +++ b/rcl/test/rcl/test_subscription.cpp @@ -204,11 +204,9 @@ TEST_F(CLASSNAME(TestSubscriptionFixture, RMW_IMPLEMENTATION), test_subscription { auto mock = mocking_utils::inject_on_return("lib:rcl", rcutils_string_map_fini, RCUTILS_RET_ERROR); - fprintf(stderr, "dubious 1\n"); ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_ERROR, ret); rcl_reset_error(); - fprintf(stderr, "end dubious 1\n"); } { rmw_ret_t rmw_validate_full_topic_name_returns = RMW_RET_OK; @@ -218,18 +216,14 @@ TEST_F(CLASSNAME(TestSubscriptionFixture, RMW_IMPLEMENTATION), test_subscription *result = RMW_TOPIC_INVALID_TOO_LONG; return rmw_validate_full_topic_name_returns; }); - fprintf(stderr, "dubious 2\n"); ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_TOPIC_NAME_INVALID, ret); rcl_reset_error(); - fprintf(stderr, "end dubious 2\n"); - fprintf(stderr, "dubious 3\n"); rmw_validate_full_topic_name_returns = RMW_RET_ERROR; ret = rcl_subscription_init(&subscription, this->node_ptr, ts, topic, &subscription_options); EXPECT_EQ(RCL_RET_ERROR, ret); rcl_reset_error(); - fprintf(stderr, "end dubious 3\n"); } { auto mock = From acf705d1e5c9dc3038317d64b00ce77a3bfd45a2 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 15:35:53 -0300 Subject: [PATCH 10/17] Make test_client pass Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index d226186cf..7807f052e 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -83,9 +83,9 @@ rcl_client_init( true, &remapped_service_name); if (ret != RCL_RET_OK) { - if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { + if (ret == RCL_RET_SERVICE_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { ret = RCL_RET_SERVICE_NAME_INVALID; - } else { + } else if (RCL_RET_BAD_ALLOC != ret) { ret = RCL_RET_ERROR; } goto cleanup; From 2965e3a811f206acbb843833176e1c75b24fc54a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 15:51:14 -0300 Subject: [PATCH 11/17] Test for rcl_node_resolve_name Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_node.cpp | 79 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index 2d20306fd..8e4333985 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -32,6 +32,7 @@ #include "rcl/logging.h" #include "rcl/logging_rosout.h" +#include "../arg_macros.hpp" #include "../mocking_utils/patch.hpp" #ifdef RMW_IMPLEMENTATION @@ -900,3 +901,81 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_options_fai EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&prev_ini_options.arguments)); } + +/* Tests special case node_options + */ +TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_name) { + rcl_allocator_t default_allocator = rcl_get_default_allocator(); + char * final_name = NULL; + rcl_node_t node = rcl_get_zero_initialized_node(); + // Invalid node + EXPECT_EQ( + RCL_RET_INVALID_ARGUMENT, + rcl_node_resolve_name(NULL, "my_topic", default_allocator, false, &final_name)); + EXPECT_EQ( + RCL_RET_ERROR, + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, &final_name)); + + // Initialize rcl with rcl_init(). + rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); + ret = rcl_init_options_init(&init_options, rcl_get_default_allocator()); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options)) << rcl_get_error_string().str; + }); + rcl_context_t context = rcl_get_zero_initialized_context(); + ret = rcl_init(0, nullptr, &init_options, &context); + ASSERT_EQ(RCL_RET_OK, ret); + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + ASSERT_EQ(RCL_RET_OK, rcl_shutdown(&context)); + ASSERT_EQ(RCL_RET_OK, rcl_context_fini(&context)); + }); + + // Initialize node with default options + rcl_node_options_t options = rcl_node_get_default_options(); + rcl_arguments_t local_arguments; + SCOPE_ARGS(local_arguments, "process_name", "--ros-args", "-r", "/bar/foo:=/foo/local_args"); + options.arguments = local_arguments; + ret = rcl_node_init(&node, "node", "/ns", &context, &default_options); + ASSERT_EQ(RCL_RET_OK, ret); + + // Invalid arguments + EXPECT_EQ( + RCL_RET_INVALID_ARGUMENT, + rcl_node_resolve_name(&node, NULL, default_allocator, false, &final_name)); + EXPECT_EQ( + RCL_RET_INVALID_ARGUMENT, + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, NULL)); + + // Some valid options, test_remap and test_expand_topic_name already have good coverage + EXPECT_EQ( + RCL_RET_OK, + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, &final_name)); + ASSERT_TRUE(final_name); + EXPECT_STREQ("/ns/my_topic", *final_name); + default_allocator.deallocate(final_name, default_allocator.state); + + EXPECT_EQ( + RCL_RET_OK, + rcl_node_resolve_name(&node, "my_service", default_allocator, true, &final_name)); + ASSERT_TRUE(final_name); + EXPECT_STREQ("/ns/my_service", *final_name); + default_allocator.deallocate(final_name, default_allocator.state); + + EXPECT_EQ( + RCL_RET_OK, + rcl_node_resolve_name(&node, "/bar/foo", default_allocator, false, &final_name)); + ASSERT_TRUE(final_name); + EXPECT_STREQ("/foo/local_args", *final_name); + default_allocator.deallocate(final_name, default_allocator.state); + + EXPECT_EQ( + RCL_RET_OK, + rcl_node_resolve_name(&node, "relative_ns/foo", default_allocator, true, &final_name)); + ASSERT_TRUE(final_name); + EXPECT_STREQ("/ns/relative_ns/foo", *final_name); + default_allocator.deallocate(final_name, default_allocator.state); +} + From 6b04c495289901faa650231a14cff4d625321c2b Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 15:55:38 -0300 Subject: [PATCH 12/17] Add new tests Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_node.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index 8e4333985..a55d09384 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -32,8 +32,8 @@ #include "rcl/logging.h" #include "rcl/logging_rosout.h" -#include "../arg_macros.hpp" #include "../mocking_utils/patch.hpp" +#include "./arg_macros.hpp" #ifdef RMW_IMPLEMENTATION # define CLASSNAME_(NAME, SUFFIX) NAME ## __ ## SUFFIX @@ -918,7 +918,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam // Initialize rcl with rcl_init(). rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); - ret = rcl_init_options_init(&init_options, rcl_get_default_allocator()); + rcl_ret_t ret = rcl_init_options_init(&init_options, rcl_get_default_allocator()); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -938,7 +938,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam rcl_arguments_t local_arguments; SCOPE_ARGS(local_arguments, "process_name", "--ros-args", "-r", "/bar/foo:=/foo/local_args"); options.arguments = local_arguments; - ret = rcl_node_init(&node, "node", "/ns", &context, &default_options); + ret = rcl_node_init(&node, "node", "/ns", &context, &options); ASSERT_EQ(RCL_RET_OK, ret); // Invalid arguments @@ -954,28 +954,28 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam RCL_RET_OK, rcl_node_resolve_name(&node, "my_topic", default_allocator, false, &final_name)); ASSERT_TRUE(final_name); - EXPECT_STREQ("/ns/my_topic", *final_name); + EXPECT_STREQ("/ns/my_topic", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, rcl_node_resolve_name(&node, "my_service", default_allocator, true, &final_name)); ASSERT_TRUE(final_name); - EXPECT_STREQ("/ns/my_service", *final_name); + EXPECT_STREQ("/ns/my_service", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, rcl_node_resolve_name(&node, "/bar/foo", default_allocator, false, &final_name)); ASSERT_TRUE(final_name); - EXPECT_STREQ("/foo/local_args", *final_name); + EXPECT_STREQ("/foo/local_args", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, rcl_node_resolve_name(&node, "relative_ns/foo", default_allocator, true, &final_name)); ASSERT_TRUE(final_name); - EXPECT_STREQ("/ns/relative_ns/foo", *final_name); + EXPECT_STREQ("/ns/relative_ns/foo", final_name); default_allocator.deallocate(final_name, default_allocator.state); } From b708b3a056bc3827b276859acb1b322758e9000b Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 16:08:13 -0300 Subject: [PATCH 13/17] fix Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/node.h | 2 +- rcl/src/rcl/node_resolve_name.c | 2 +- rcl/test/rcl/test_node.cpp | 18 +++++++++++++++--- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index f79e48f96..a8c9820d8 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -496,7 +496,7 @@ rcl_node_get_logger_name(const rcl_node_t * node); * Attribute | Adherence * ------------------ | ------------- * Allocates Memory | Yes - * Thread-Safe | Yes + * Thread-Safe | No * Uses Atomics | No * Lock-Free | Yes * diff --git a/rcl/src/rcl/node_resolve_name.c b/rcl/src/rcl/node_resolve_name.c index 1660e3886..2a281384d 100644 --- a/rcl/src/rcl/node_resolve_name.c +++ b/rcl/src/rcl/node_resolve_name.c @@ -135,7 +135,7 @@ rcl_node_resolve_name( bool is_service, char ** output_topic_name) { - // node get options checks `node` validity + RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); const rcl_node_options_t * node_options = rcl_node_get_options(node); if (NULL == node_options) { return RCL_RET_ERROR; diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index a55d09384..9e644ba4b 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -935,11 +935,23 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam // Initialize node with default options rcl_node_options_t options = rcl_node_get_default_options(); - rcl_arguments_t local_arguments; - SCOPE_ARGS(local_arguments, "process_name", "--ros-args", "-r", "/bar/foo:=/foo/local_args"); - options.arguments = local_arguments; + rcl_arguments_t local_arguments = rcl_get_zero_initialized_arguments(); + const char * argv[] = {"process_name", "--ros-args", "-r", "/bar/foo:=/foo/local_args"}; + unsigned int argc = (sizeof(argv) / sizeof(const char *)); + ret = rcl_parse_arguments( + argc, argv, default_allocator, &local_arguments); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + options.arguments = local_arguments; // transfer ownership + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + rcl_node_options_fini(&options); + }); ret = rcl_node_init(&node, "node", "/ns", &context, &options); ASSERT_EQ(RCL_RET_OK, ret); + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + ASSERT_EQ(RCL_RET_OK, rcl_node_fini(&node)); + }); // Invalid arguments EXPECT_EQ( From 4af74fca62e6c01b92a7ff8cdd9921602d44e3be Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 16:30:28 -0300 Subject: [PATCH 14/17] Readd only expand, can be useful in rclcpp Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/node.h | 4 +++- rcl/src/rcl/client.c | 1 + rcl/src/rcl/node_resolve_name.c | 17 +++++++++++------ rcl/src/rcl/publisher.c | 1 + rcl/src/rcl/service.c | 1 + rcl/src/rcl/subscription.c | 1 + rcl/test/rcl/test_node.cpp | 23 +++++++++++++++-------- 7 files changed, 33 insertions(+), 15 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index a8c9820d8..011347fd3 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -503,7 +503,8 @@ rcl_node_get_logger_name(const rcl_node_t * node); * \param[in] node Node object. Its name, namespace, local/global command line arguments are used. * \param[in] input_name Topic name to be expanded and remapped. * \param[in] allocator The allocator to be used when creating the output topic. - * \param[in] is_service for services use `true`, for topics use `false`. + * \param[in] is_service For services use `true`, for topics use `false`. + * \param[in] only_expand When `true`, remapping rules are ignored. * \param[out] output_name Output char * pointer. * \return `RCL_RET_OK` if the topic name was expanded successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any of input_name, node_name, node_namespace @@ -527,6 +528,7 @@ rcl_node_resolve_name( const char * input_name, rcl_allocator_t allocator, bool is_service, + bool only_expand, char ** output_name); #ifdef __cplusplus diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 7807f052e..53dc77ca1 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -81,6 +81,7 @@ rcl_client_init( service_name, *allocator, true, + false, &remapped_service_name); if (ret != RCL_RET_OK) { if (ret == RCL_RET_SERVICE_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { diff --git a/rcl/src/rcl/node_resolve_name.c b/rcl/src/rcl/node_resolve_name.c index 2a281384d..2c493ca6e 100644 --- a/rcl/src/rcl/node_resolve_name.c +++ b/rcl/src/rcl/node_resolve_name.c @@ -37,6 +37,7 @@ rcl_resolve_name( const char * node_namespace, rcl_allocator_t allocator, bool is_service, + bool only_expand, char ** output_topic_name) { // the other arguments are checked by rcl_expand_topic_name() and rcl_remap_topic_name() @@ -74,12 +75,14 @@ rcl_resolve_name( goto cleanup; } // remap topic name - ret = rcl_remap_name( - local_args, global_args, is_service ? RCL_SERVICE_REMAP : RCL_TOPIC_REMAP, - expanded_topic_name, node_name, node_namespace, &substitutions_map, allocator, - &remapped_topic_name); - if (RCL_RET_OK != ret) { - goto cleanup; + if (!only_expand) { + ret = rcl_remap_name( + local_args, global_args, is_service ? RCL_SERVICE_REMAP : RCL_TOPIC_REMAP, + expanded_topic_name, node_name, node_namespace, &substitutions_map, allocator, + &remapped_topic_name); + if (RCL_RET_OK != ret) { + goto cleanup; + } } if (NULL == remapped_topic_name) { remapped_topic_name = expanded_topic_name; @@ -133,6 +136,7 @@ rcl_node_resolve_name( const char * input_topic_name, rcl_allocator_t allocator, bool is_service, + bool only_expand, char ** output_topic_name) { RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); @@ -153,5 +157,6 @@ rcl_node_resolve_name( rcl_node_get_namespace(node), allocator, is_service, + only_expand, output_topic_name); } diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 71291ae0f..3046598a3 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -83,6 +83,7 @@ rcl_publisher_init( topic_name, *allocator, false, + false, &remapped_topic_name); if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index e1c26f4a7..251ab66b1 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -85,6 +85,7 @@ rcl_service_init( service_name, *allocator, true, + false, &remapped_service_name); if (ret != RCL_RET_OK) { if (ret == RCL_RET_SERVICE_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 60c79d550..5b122e415 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -74,6 +74,7 @@ rcl_subscription_init( topic_name, *allocator, false, + false, &remapped_topic_name); if (ret != RCL_RET_OK) { if (ret == RCL_RET_TOPIC_NAME_INVALID || ret == RCL_RET_UNKNOWN_SUBSTITUTION) { diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index 9e644ba4b..dc7cea660 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -911,10 +911,10 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam // Invalid node EXPECT_EQ( RCL_RET_INVALID_ARGUMENT, - rcl_node_resolve_name(NULL, "my_topic", default_allocator, false, &final_name)); + rcl_node_resolve_name(NULL, "my_topic", default_allocator, false, false, &final_name)); EXPECT_EQ( RCL_RET_ERROR, - rcl_node_resolve_name(&node, "my_topic", default_allocator, false, &final_name)); + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, false, &final_name)); // Initialize rcl with rcl_init(). rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); @@ -956,36 +956,43 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam // Invalid arguments EXPECT_EQ( RCL_RET_INVALID_ARGUMENT, - rcl_node_resolve_name(&node, NULL, default_allocator, false, &final_name)); + rcl_node_resolve_name(&node, NULL, default_allocator, false, false, &final_name)); EXPECT_EQ( RCL_RET_INVALID_ARGUMENT, - rcl_node_resolve_name(&node, "my_topic", default_allocator, false, NULL)); + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, false, NULL)); // Some valid options, test_remap and test_expand_topic_name already have good coverage EXPECT_EQ( RCL_RET_OK, - rcl_node_resolve_name(&node, "my_topic", default_allocator, false, &final_name)); + rcl_node_resolve_name(&node, "my_topic", default_allocator, false, false, &final_name)); ASSERT_TRUE(final_name); EXPECT_STREQ("/ns/my_topic", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, - rcl_node_resolve_name(&node, "my_service", default_allocator, true, &final_name)); + rcl_node_resolve_name(&node, "my_service", default_allocator, true, false, &final_name)); ASSERT_TRUE(final_name); EXPECT_STREQ("/ns/my_service", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, - rcl_node_resolve_name(&node, "/bar/foo", default_allocator, false, &final_name)); + rcl_node_resolve_name(&node, "/bar/foo", default_allocator, false, false, &final_name)); ASSERT_TRUE(final_name); EXPECT_STREQ("/foo/local_args", final_name); default_allocator.deallocate(final_name, default_allocator.state); EXPECT_EQ( RCL_RET_OK, - rcl_node_resolve_name(&node, "relative_ns/foo", default_allocator, true, &final_name)); + rcl_node_resolve_name(&node, "/bar/foo", default_allocator, false, true, &final_name)); + ASSERT_TRUE(final_name); + EXPECT_STREQ("/bar/foo", final_name); + default_allocator.deallocate(final_name, default_allocator.state); + + EXPECT_EQ( + RCL_RET_OK, + rcl_node_resolve_name(&node, "relative_ns/foo", default_allocator, true, false, &final_name)); ASSERT_TRUE(final_name); EXPECT_STREQ("/ns/relative_ns/foo", final_name); default_allocator.deallocate(final_name, default_allocator.state); From e296b8c936629e720534bd2dd4dbc66918d742b0 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 16:35:32 -0300 Subject: [PATCH 15/17] please linters Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_node.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index dc7cea660..c21576126 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -997,4 +997,3 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_resolve_nam EXPECT_STREQ("/ns/relative_ns/foo", final_name); default_allocator.deallocate(final_name, default_allocator.state); } - From a94f2db1fad18abfd0e84c2d7afc87abaa341cb7 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 16 Oct 2020 18:51:55 -0300 Subject: [PATCH 16/17] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rcl/src/rcl/node_resolve_name.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/node_resolve_name.c b/rcl/src/rcl/node_resolve_name.c index 2c493ca6e..92f12ae79 100644 --- a/rcl/src/rcl/node_resolve_name.c +++ b/rcl/src/rcl/node_resolve_name.c @@ -40,7 +40,7 @@ rcl_resolve_name( bool only_expand, char ** output_topic_name) { - // the other arguments are checked by rcl_expand_topic_name() and rcl_remap_topic_name() + // the other arguments are checked by rcl_expand_topic_name() and rcl_remap_name() RCL_CHECK_ARGUMENT_FOR_NULL(output_topic_name, RCL_RET_INVALID_ARGUMENT); // Create default topic name substitutions map rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); From 84681f1cd9271bb32efd5855bf9f19209a984a5b Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 19 Oct 2020 10:26:28 -0300 Subject: [PATCH 17/17] Memory safety 101 Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/node_resolve_name.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/src/rcl/node_resolve_name.c b/rcl/src/rcl/node_resolve_name.c index 92f12ae79..9c2667669 100644 --- a/rcl/src/rcl/node_resolve_name.c +++ b/rcl/src/rcl/node_resolve_name.c @@ -54,6 +54,8 @@ rcl_resolve_name( } return RCL_RET_ERROR; } + char * expanded_topic_name = NULL; + char * remapped_topic_name = NULL; rcl_ret_t ret = rcl_get_default_topic_name_substitutions(&substitutions_map); if (ret != RCL_RET_OK) { if (RCL_RET_BAD_ALLOC != ret) { @@ -62,8 +64,6 @@ rcl_resolve_name( goto cleanup; } // expand topic name - char * expanded_topic_name = NULL; - char * remapped_topic_name = NULL; ret = rcl_expand_topic_name( input_topic_name, node_name,