From f409064b74b6a6e9e3deeb37c873c333a01634f5 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 21 Jul 2020 16:33:57 -0300 Subject: [PATCH] Ensure compliant pub construction/destruction API. Signed-off-by: Michel Hidalgo --- rmw_fastrtps_cpp/src/publisher.cpp | 87 +++++++++++-------- rmw_fastrtps_cpp/src/rmw_publisher.cpp | 30 +++++-- rmw_fastrtps_dynamic_cpp/src/publisher.cpp | 86 +++++++++--------- .../src/rmw_publisher.cpp | 34 +++++--- rmw_fastrtps_shared_cpp/src/publisher.cpp | 32 ++----- rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp | 25 +----- 6 files changed, 150 insertions(+), 144 deletions(-) diff --git a/rmw_fastrtps_cpp/src/publisher.cpp b/rmw_fastrtps_cpp/src/publisher.cpp index c83ed5c89..0bc364320 100644 --- a/rmw_fastrtps_cpp/src/publisher.cpp +++ b/rmw_fastrtps_cpp/src/publisher.cpp @@ -21,6 +21,9 @@ #include "rmw/allocators.h" #include "rmw/error_handling.h" #include "rmw/rmw.h" +#include "rmw/validate_full_topic_name.h" + +#include "rcpputils/scope_exit.hpp" #include "rmw_fastrtps_shared_cpp/create_rmw_gid.hpp" #include "rmw_fastrtps_shared_cpp/custom_participant_info.hpp" @@ -49,20 +52,27 @@ rmw_fastrtps_cpp::create_publisher( bool keyed, bool create_publisher_listener) { - if (!participant_info) { - RMW_SET_ERROR_MSG("participant_info is null"); - return nullptr; - } - if (!topic_name || strlen(topic_name) == 0) { - RMW_SET_ERROR_MSG("publisher topic is null or empty string"); - return nullptr; - } - if (!qos_policies) { - RMW_SET_ERROR_MSG("qos_policies is null"); - return nullptr; + RMW_CHECK_ARGUMENT_FOR_NULL(participant_info, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(type_supports, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(topic_name, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(qos_policies, nullptr); + if (!qos_policies->avoid_ros_namespace_conventions) { + int validation_result = RMW_TOPIC_VALID; + rmw_ret_t ret = rmw_validate_full_topic_name(topic_name, &validation_result, nullptr); + if (RMW_RET_OK != ret) { + return nullptr; + } + if (RMW_TOPIC_VALID != validation_result) { + const char * reason = rmw_full_topic_name_validation_result_string(validation_result); + RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid topic name: %s", reason); + return nullptr; + } } - if (!publisher_options) { - RMW_SET_ERROR_MSG("publisher_options is null"); + RMW_CHECK_ARGUMENT_FOR_NULL(publisher_options, nullptr); + + Participant * participant = participant_info->participant; + if (!participant) { + RMW_SET_ERROR_MSG("participant handle is null"); return nullptr; } @@ -93,7 +103,14 @@ rmw_fastrtps_cpp::create_publisher( RMW_SET_ERROR_MSG("failed to allocate CustomPublisherInfo"); return nullptr; } - + auto cleanup_info = rcpputils::make_scope_exit( + [info, participant]() { + if (info->type_support_) { + _unregister_type(participant, info->type_support_); + } + delete info->listener_; + delete info; + }); info->typesupport_identifier_ = type_support->typesupport_identifier; info->type_support_impl_ = type_support->data; @@ -101,15 +118,15 @@ rmw_fastrtps_cpp::create_publisher( std::string type_name = _create_type_name(callbacks); if ( !Domain::getRegisteredType( - participant_info->participant, type_name.c_str(), + participant, type_name.c_str(), reinterpret_cast(&info->type_support_))) { info->type_support_ = new (std::nothrow) MessageTypeSupport_cpp(callbacks); if (!info->type_support_) { - RMW_SET_ERROR_MSG("Failed to allocate MessageTypeSupport"); - goto fail; + RMW_SET_ERROR_MSG("failed to allocate MessageTypeSupport"); + return nullptr; } - _register_type(participant_info->participant, info->type_support_); + _register_type(participant, info->type_support_); } if (!participant_info->leave_middleware_default_qos) { @@ -124,8 +141,7 @@ rmw_fastrtps_cpp::create_publisher( publisherParam.topic.topicName = _create_topic_name(qos_policies, ros_topic_prefix, topic_name); if (!get_datawriter_qos(*qos_policies, publisherParam)) { - RMW_SET_ERROR_MSG("failed to get datawriter qos"); - goto fail; + return nullptr; } info->listener_ = nullptr; @@ -133,17 +149,17 @@ rmw_fastrtps_cpp::create_publisher( info->listener_ = new (std::nothrow) PubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); - goto fail; + return nullptr; } } info->publisher_ = Domain::createPublisher( - participant_info->participant, + participant, publisherParam, info->listener_); if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); - goto fail; + return nullptr; } info->publisher_gid = rmw_fastrtps_shared_cpp::create_rmw_gid( @@ -152,30 +168,27 @@ rmw_fastrtps_cpp::create_publisher( rmw_publisher = rmw_publisher_allocate(); if (!rmw_publisher) { RMW_SET_ERROR_MSG("failed to allocate publisher"); - goto fail; + return nullptr; } + auto cleanup_publisher = rcpputils::make_scope_exit( + [rmw_publisher]() { + rmw_free(const_cast(rmw_publisher->topic_name)); + rmw_publisher_free(rmw_publisher); + }); + rmw_publisher->implementation_identifier = eprosima_fastrtps_identifier; rmw_publisher->data = info; - rmw_publisher->topic_name = static_cast(rmw_allocate(strlen(topic_name) + 1)); + rmw_publisher->topic_name = static_cast(rmw_allocate(strlen(topic_name) + 1)); if (!rmw_publisher->topic_name) { RMW_SET_ERROR_MSG("failed to allocate memory for publisher topic name"); - goto fail; + return nullptr; } - memcpy(const_cast(rmw_publisher->topic_name), topic_name, strlen(topic_name) + 1); rmw_publisher->options = *publisher_options; + cleanup_publisher.cancel(); + cleanup_info.cancel(); return rmw_publisher; - -fail: - if (info) { - delete info->type_support_; - delete info->listener_; - delete info; - } - rmw_publisher_free(rmw_publisher); - - return nullptr; } diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp index 98d9079ed..a3b50cbe3 100644 --- a/rmw_fastrtps_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_cpp/src/rmw_publisher.cpp @@ -19,6 +19,8 @@ #include "rmw/error_handling.h" #include "rmw/rmw.h" +#include "rmw/impl/cpp/macros.hpp" + #include "rmw_fastrtps_shared_cpp/custom_participant_info.hpp" #include "rmw_fastrtps_shared_cpp/custom_publisher_info.hpp" #include "rmw_fastrtps_shared_cpp/rmw_common.hpp" @@ -63,15 +65,12 @@ rmw_create_publisher( const rmw_qos_profile_t * qos_policies, const rmw_publisher_options_t * publisher_options) { - if (!node) { - RMW_SET_ERROR_MSG("node handle is null"); - return nullptr; - } - - if (node->implementation_identifier != eprosima_fastrtps_identifier) { - RMW_SET_ERROR_MSG("node handle not from this implementation"); - return nullptr; - } + RMW_CHECK_ARGUMENT_FOR_NULL(node, nullptr); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return nullptr); rmw_publisher_t * publisher = rmw_fastrtps_cpp::create_publisher( static_cast(node->context->impl->participant_info), @@ -164,6 +163,19 @@ rmw_return_loaned_message_from_publisher( rmw_ret_t rmw_destroy_publisher(rmw_node_t * node, rmw_publisher_t * publisher) { + RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_ARGUMENT_FOR_NULL(publisher, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + publisher, + publisher->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); + return rmw_fastrtps_shared_cpp::__rmw_destroy_publisher( eprosima_fastrtps_identifier, node, publisher); } diff --git a/rmw_fastrtps_dynamic_cpp/src/publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/publisher.cpp index 6e5a6ad2c..953b1bcd4 100644 --- a/rmw_fastrtps_dynamic_cpp/src/publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/publisher.cpp @@ -18,6 +18,9 @@ #include "rmw/allocators.h" #include "rmw/error_handling.h" #include "rmw/rmw.h" +#include "rmw/validate_full_topic_name.h" + +#include "rcpputils/scope_exit.hpp" #include "rmw_fastrtps_shared_cpp/custom_participant_info.hpp" #include "rmw_fastrtps_shared_cpp/custom_publisher_info.hpp" @@ -51,25 +54,23 @@ rmw_fastrtps_dynamic_cpp::create_publisher( (void)keyed; (void)create_publisher_listener; - if (!participant_info) { - RMW_SET_ERROR_MSG("participant_info is null"); - return nullptr; - } - - if (!topic_name || strlen(topic_name) == 0) { - RMW_SET_ERROR_MSG("publisher topic is null or empty string"); - return nullptr; - } - - if (!qos_policies) { - RMW_SET_ERROR_MSG("qos_policies is null"); - return nullptr; - } - - if (!publisher_options) { - RMW_SET_ERROR_MSG("publisher_options is null"); - return nullptr; + RMW_CHECK_ARGUMENT_FOR_NULL(participant_info, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(type_supports, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(topic_name, nullptr); + RMW_CHECK_ARGUMENT_FOR_NULL(qos_policies, nullptr); + if (!qos_policies->avoid_ros_namespace_conventions) { + int validation_result = RMW_TOPIC_VALID; + rmw_ret_t ret = rmw_validate_full_topic_name(topic_name, &validation_result, nullptr); + if (RMW_RET_OK != ret) { + return nullptr; + } + if (RMW_TOPIC_VALID != validation_result) { + const char * reason = rmw_full_topic_name_validation_result_string(validation_result); + RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("invalid topic name: %s", reason); + return nullptr; + } } + RMW_CHECK_ARGUMENT_FOR_NULL(publisher_options, nullptr); Participant * participant = participant_info->participant; if (!participant) { @@ -104,14 +105,25 @@ rmw_fastrtps_dynamic_cpp::create_publisher( RMW_SET_ERROR_MSG("failed to allocate CustomPublisherInfo"); return nullptr; } + auto cleanup_info = rcpputils::make_scope_exit( + [info, participant]() { + if (info->type_support_) { + _unregister_type(participant, info->type_support_); + } + delete info->listener_; + delete info; + }); TypeSupportRegistry & type_registry = TypeSupportRegistry::get_instance(); auto type_impl = type_registry.get_message_type_support(type_support); if (!type_impl) { - delete info; RMW_SET_ERROR_MSG("failed to allocate type support"); return nullptr; } + auto return_type_support = rcpputils::make_scope_exit( + [&type_registry, type_support]() { + type_registry.return_message_type_support(type_support); + }); info->typesupport_identifier_ = type_support->typesupport_identifier; info->type_support_impl_ = type_impl; @@ -126,7 +138,7 @@ rmw_fastrtps_dynamic_cpp::create_publisher( info->type_support_ = new (std::nothrow) TypeSupportProxy(type_impl); if (!info->type_support_) { RMW_SET_ERROR_MSG("failed to allocate TypeSupportProxy"); - goto fail; + return nullptr; } _register_type(participant, info->type_support_); } @@ -150,20 +162,19 @@ rmw_fastrtps_dynamic_cpp::create_publisher( // publisherParam.throughputController = throughputController; if (!get_datawriter_qos(*qos_policies, publisherParam)) { - RMW_SET_ERROR_MSG("failed to get datawriter qos"); - goto fail; + return nullptr; } info->listener_ = new (std::nothrow) PubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); - goto fail; + return nullptr; } info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); - goto fail; + return nullptr; } info->publisher_gid = rmw_fastrtps_shared_cpp::create_rmw_gid( @@ -172,32 +183,29 @@ rmw_fastrtps_dynamic_cpp::create_publisher( rmw_publisher = rmw_publisher_allocate(); if (!rmw_publisher) { RMW_SET_ERROR_MSG("failed to allocate publisher"); - goto fail; + return nullptr; } + auto cleanup_publisher = rcpputils::make_scope_exit( + [rmw_publisher]() { + rmw_free(const_cast(rmw_publisher->topic_name)); + rmw_publisher_free(rmw_publisher); + }); + rmw_publisher->can_loan_messages = false; rmw_publisher->implementation_identifier = eprosima_fastrtps_identifier; rmw_publisher->data = info; - rmw_publisher->topic_name = static_cast(rmw_allocate(strlen(topic_name) + 1)); + rmw_publisher->topic_name = static_cast(rmw_allocate(strlen(topic_name) + 1)); if (!rmw_publisher->topic_name) { RMW_SET_ERROR_MSG("failed to allocate memory for publisher topic name"); - goto fail; + return nullptr; } - memcpy(const_cast(rmw_publisher->topic_name), topic_name, strlen(topic_name) + 1); rmw_publisher->options = *publisher_options; + cleanup_publisher.cancel(); + cleanup_info.cancel(); + return_type_support.cancel(); return rmw_publisher; - -fail: - if (info) { - delete info->type_support_; - delete info->listener_; - delete info; - } - type_registry.return_message_type_support(type_support); - rmw_publisher_free(rmw_publisher); - - return nullptr; } diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp index 45e2ce135..e5c948319 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp @@ -18,6 +18,8 @@ #include "rmw/error_handling.h" #include "rmw/rmw.h" +#include "rmw/impl/cpp/macros.hpp" + #include "rmw_fastrtps_shared_cpp/custom_participant_info.hpp" #include "rmw_fastrtps_shared_cpp/custom_publisher_info.hpp" #include "rmw_fastrtps_shared_cpp/rmw_common.hpp" @@ -65,15 +67,12 @@ rmw_create_publisher( const rmw_qos_profile_t * qos_policies, const rmw_publisher_options_t * publisher_options) { - if (!node) { - RMW_SET_ERROR_MSG("node handle is null"); - return nullptr; - } - - if (node->implementation_identifier != eprosima_fastrtps_identifier) { - RMW_SET_ERROR_MSG("node handle not from this implementation"); - return nullptr; - } + RMW_CHECK_ARGUMENT_FOR_NULL(node, nullptr); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return nullptr); auto impl = static_cast(node->context->impl->participant_info); @@ -170,12 +169,21 @@ using BaseTypeSupport = rmw_fastrtps_dynamic_cpp::BaseTypeSupport; rmw_ret_t rmw_destroy_publisher(rmw_node_t * node, rmw_publisher_t * publisher) { - auto info = static_cast(publisher->data); - RCUTILS_CHECK_FOR_NULL_WITH_MSG(info, "publisher info pointer is null", return RMW_RET_ERROR); + RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_ARGUMENT_FOR_NULL(publisher, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + node, + node->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); + RMW_CHECK_TYPE_IDENTIFIERS_MATCH( + publisher, + publisher->implementation_identifier, + eprosima_fastrtps_identifier, + return RMW_RET_INCORRECT_RMW_IMPLEMENTATION); + auto info = static_cast(publisher->data); auto impl = static_cast(info->type_support_impl_); - RCUTILS_CHECK_FOR_NULL_WITH_MSG(impl, "publisher type support is null", return RMW_RET_ERROR); - auto ros_type_support = static_cast( impl->ros_type_support()); diff --git a/rmw_fastrtps_shared_cpp/src/publisher.cpp b/rmw_fastrtps_shared_cpp/src/publisher.cpp index 9dc3eb3d7..03c6f5cb0 100644 --- a/rmw_fastrtps_shared_cpp/src/publisher.cpp +++ b/rmw_fastrtps_shared_cpp/src/publisher.cpp @@ -35,33 +35,17 @@ destroy_publisher( CustomParticipantInfo * participant_info, rmw_publisher_t * publisher) { - if (!publisher) { - RMW_SET_ERROR_MSG("publisher handle is null"); - return RMW_RET_ERROR; - } - if (publisher->implementation_identifier != identifier) { - RMW_SET_ERROR_MSG("publisher handle not from this implementation"); - return RMW_RET_ERROR; - } - if (!participant_info) { - RMW_SET_ERROR_MSG("participant_info is null"); - return RMW_RET_ERROR; - } + assert(publisher->implementation_identifier == identifier); + static_cast(identifier); auto info = static_cast(publisher->data); - if (info != nullptr) { - if (info->publisher_ != nullptr) { - Domain::removePublisher(info->publisher_); - } - delete info->listener_; - if (info->type_support_ != nullptr) { - Participant * participant = participant_info->participant; - _unregister_type(participant, info->type_support_); - } - delete info; - } + Domain::removePublisher(info->publisher_); + delete info->listener_; + + _unregister_type(participant_info->participant, info->type_support_); + delete info; + rmw_free(const_cast(publisher->topic_name)); - publisher->topic_name = nullptr; rmw_publisher_free(publisher); return RMW_RET_OK; diff --git a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp index 99b45bac1..3dd7ce059 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp @@ -37,24 +37,8 @@ __rmw_destroy_publisher( const rmw_node_t * node, rmw_publisher_t * publisher) { - if (!node) { - RMW_SET_ERROR_MSG("node handle is null"); - return RMW_RET_ERROR; - } - - if (node->implementation_identifier != identifier) { - RMW_SET_ERROR_MSG("publisher handle not from this implementation"); - return RMW_RET_ERROR; - } - - if (!publisher) { - RMW_SET_ERROR_MSG("publisher handle is null"); - return RMW_RET_ERROR; - } - if (publisher->implementation_identifier != identifier) { - RMW_SET_ERROR_MSG("publisher handle not from this implementation"); - return RMW_RET_ERROR; - } + assert(node->implementation_identifier == identifier); + assert(publisher->implementation_identifier == identifier); auto common_context = static_cast(node->context->impl->common); auto info = static_cast(publisher->data); @@ -76,10 +60,7 @@ __rmw_destroy_publisher( auto participant_info = static_cast(node->context->impl->participant_info); - return destroy_publisher( - identifier, - participant_info, - publisher); + return destroy_publisher(identifier, participant_info, publisher); } rmw_ret_t