From 27ac74a96140f453ef3ddb0ba351a15e0387edc7 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 15 Jul 2020 10:23:24 -0300 Subject: [PATCH] Ensure compliant node construction/destruction API. (#408) Signed-off-by: Michel Hidalgo --- rmw_fastrtps_cpp/src/rmw_node.cpp | 18 ++++- rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp | 19 ++++- rmw_fastrtps_shared_cpp/src/rmw_node.cpp | 91 ++++++++++++----------- 3 files changed, 80 insertions(+), 48 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index 3aae8dac9..e3daad4e0 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -28,6 +28,7 @@ #include "rmw_fastrtps_shared_cpp/init_rmw_context_impl.hpp" #include "rmw_fastrtps_shared_cpp/rmw_common.hpp" +#include "rmw_fastrtps_shared_cpp/rmw_context_impl.hpp" #include "rmw_fastrtps_cpp/identifier.hpp" #include "rmw_fastrtps_cpp/init_rmw_context_impl.hpp" @@ -44,13 +45,21 @@ rmw_create_node( { (void)domain_id; (void)localhost_only; - RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, NULL); + RMW_CHECK_ARGUMENT_FOR_NULL(context, nullptr); RMW_CHECK_TYPE_IDENTIFIERS_MATCH( init context, context->implementation_identifier, eprosima_fastrtps_identifier, // TODO(wjwwood): replace this with RMW_RET_INCORRECT_RMW_IMPLEMENTATION when refactored return nullptr); + RMW_CHECK_FOR_NULL_WITH_MSG( + context->impl, + "expected initialized context", + return nullptr); + if (context->impl->is_shutdown) { + RCUTILS_SET_ERROR_MSG("context has been shutdown"); + return nullptr; + } if (RMW_RET_OK != rmw_fastrtps_cpp::increment_context_impl_ref_count(context)) { return nullptr; @@ -72,6 +81,13 @@ rmw_create_node( rmw_ret_t rmw_destroy_node(rmw_node_t * node) { + RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); + rmw_context_t * context = node->context; rmw_ret_t ret = rmw_fastrtps_shared_cpp::__rmw_destroy_node( eprosima_fastrtps_identifier, node); diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp index 6c8e90890..84bd9de11 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_node.cpp @@ -28,6 +28,7 @@ #include "rmw_fastrtps_shared_cpp/init_rmw_context_impl.hpp" #include "rmw_fastrtps_shared_cpp/rmw_common.hpp" +#include "rmw_fastrtps_shared_cpp/rmw_context_impl.hpp" #include "rmw_fastrtps_dynamic_cpp/identifier.hpp" #include "rmw_fastrtps_dynamic_cpp/init_rmw_context_impl.hpp" @@ -44,13 +45,21 @@ rmw_create_node( { (void)domain_id; (void)localhost_only; - RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, NULL); + RMW_CHECK_ARGUMENT_FOR_NULL(context, nullptr); RMW_CHECK_TYPE_IDENTIFIERS_MATCH( init context, context->implementation_identifier, eprosima_fastrtps_identifier, // TODO(wjwwood): replace this with RMW_RET_INCORRECT_RMW_IMPLEMENTATION when refactored - return NULL); + return nullptr); + RMW_CHECK_FOR_NULL_WITH_MSG( + context->impl, + "expected initialized context", + return nullptr); + if (context->impl->is_shutdown) { + RCUTILS_SET_ERROR_MSG("context has been shutdown"); + return nullptr; + } if (RMW_RET_OK != rmw_fastrtps_dynamic_cpp::increment_context_impl_ref_count(context)) { return nullptr; @@ -72,6 +81,12 @@ rmw_create_node( rmw_ret_t rmw_destroy_node(rmw_node_t * node) { + RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); rmw_context_t * context = node->context; rmw_ret_t ret = rmw_fastrtps_shared_cpp::__rmw_destroy_node( eprosima_fastrtps_identifier, node); diff --git a/rmw_fastrtps_shared_cpp/src/rmw_node.cpp b/rmw_fastrtps_shared_cpp/src/rmw_node.cpp index 14c935102..7a3a4f915 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_node.cpp @@ -25,6 +25,10 @@ #include "rmw/error_handling.h" #include "rmw/impl/cpp/macros.hpp" #include "rmw/rmw.h" +#include "rmw/validate_namespace.h" +#include "rmw/validate_node_name.h" + +#include "rcpputils/scope_exit.hpp" #include "rmw_dds_common/context.hpp" @@ -42,44 +46,61 @@ __rmw_create_node( const char * name, const char * namespace_) { - if (!name) { - RMW_SET_ERROR_MSG("name is null"); + assert(identifier == context->implementation_identifier); + + int validation_result = RMW_NODE_NAME_VALID; + rmw_ret_t ret = rmw_validate_node_name(name, &validation_result, nullptr); + if (RMW_RET_OK != ret) { return nullptr; } - - if (!namespace_) { - RMW_SET_ERROR_MSG("namespace_ is null"); + if (RMW_NODE_NAME_VALID != validation_result) { + const char * reason = rmw_node_name_validation_result_string(validation_result); + RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid node name: %s", reason); + return nullptr; + } + validation_result = RMW_NAMESPACE_VALID; + ret = rmw_validate_namespace(namespace_, &validation_result, nullptr); + if (RMW_RET_OK != ret) { + return nullptr; + } + if (RMW_NAMESPACE_VALID != validation_result) { + const char * reason = rmw_node_name_validation_result_string(validation_result); + RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid node namespace: %s", reason); return nullptr; } - rmw_node_t * node_handle = nullptr; auto common_context = static_cast(context->impl->common); rmw_dds_common::GraphCache & graph_cache = common_context->graph_cache; - - node_handle = rmw_node_allocate(); - if (!node_handle) { - RMW_SET_ERROR_MSG("failed to allocate rmw_node_t"); - goto fail; + rmw_node_t * node_handle = rmw_node_allocate(); + if (nullptr == node_handle) { + RMW_SET_ERROR_MSG("failed to allocate node"); + return nullptr; } + auto cleanup_node = rcpputils::make_scope_exit( + [node_handle]() { + rmw_free(const_cast(node_handle->name)); + rmw_free(const_cast(node_handle->namespace_)); + rmw_node_free(node_handle); + }); node_handle->implementation_identifier = identifier; node_handle->data = nullptr; node_handle->name = static_cast(rmw_allocate(sizeof(char) * strlen(name) + 1)); - if (!node_handle->name) { - RMW_SET_ERROR_MSG("failed to allocate memory"); - node_handle->namespace_ = nullptr; // to avoid free on uninitialized memory - goto fail; + if (nullptr == node_handle->name) { + RMW_SET_ERROR_MSG("failed to copy node name"); + return nullptr; } memcpy(const_cast(node_handle->name), name, strlen(name) + 1); node_handle->namespace_ = static_cast(rmw_allocate(sizeof(char) * strlen(namespace_) + 1)); - if (!node_handle->namespace_) { - RMW_SET_ERROR_MSG("failed to allocate memory"); - goto fail; + if (nullptr == node_handle->namespace_) { + RMW_SET_ERROR_MSG("failed to copy node namespace"); + return nullptr; } memcpy(const_cast(node_handle->namespace_), namespace_, strlen(namespace_) + 1); + node_handle->context = context; { @@ -92,24 +113,16 @@ __rmw_create_node( rmw_dds_common::msg::ParticipantEntitiesInfo participant_msg = graph_cache.add_node(common_context->gid, name, namespace_); if (RMW_RET_OK != __rmw_publish( - identifier, + node_handle->implementation_identifier, common_context->pub, static_cast(&participant_msg), nullptr)) { - goto fail; + return nullptr; } } + cleanup_node.cancel(); return node_handle; -fail: - if (node_handle) { - rmw_free(const_cast(node_handle->namespace_)); - node_handle->namespace_ = nullptr; - rmw_free(const_cast(node_handle->name)); - node_handle->name = nullptr; - } - rmw_node_free(node_handle); - return nullptr; } rmw_ret_t @@ -117,16 +130,7 @@ __rmw_destroy_node( const char * identifier, rmw_node_t * node) { - rmw_ret_t result_ret = RMW_RET_OK; - if (!node) { - RMW_SET_ERROR_MSG("node handle is null"); - return RMW_RET_ERROR; - } - - if (node->implementation_identifier != identifier) { - RMW_SET_ERROR_MSG("node handle not from this implementation"); - return RMW_RET_ERROR; - } + assert(node->implementation_identifier == identifier); auto common_context = static_cast(node->context->impl->common); rmw_dds_common::GraphCache & graph_cache = common_context->graph_cache; @@ -134,21 +138,18 @@ __rmw_destroy_node( std::lock_guard guard(common_context->node_update_mutex); rmw_dds_common::msg::ParticipantEntitiesInfo participant_msg = graph_cache.remove_node(common_context->gid, node->name, node->namespace_); - result_ret = __rmw_publish( + rmw_ret_t ret = __rmw_publish( identifier, common_context->pub, static_cast(&participant_msg), nullptr); - if (RMW_RET_OK != result_ret) { - return result_ret; + if (RMW_RET_OK != ret) { + return ret; } } rmw_free(const_cast(node->name)); - node->name = nullptr; rmw_free(const_cast(node->namespace_)); - node->namespace_ = nullptr; rmw_node_free(node); - return RMW_RET_OK; }