From 7e582874dfe83611d81d8e4075cf5174fe79fecf Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 8 Nov 2018 15:28:38 -0600 Subject: [PATCH 1/7] Methods to retrieve matched counts on pub/sub. --- rmw_fastrtps_cpp/src/rmw_publisher.cpp | 12 +++++- rmw_fastrtps_cpp/src/rmw_subscription.cpp | 9 +++++ .../custom_publisher_info.hpp | 39 +++++++++++++++++++ .../custom_subscriber_info.hpp | 17 +++++++- .../rmw_fastrtps_shared_cpp/rmw_common.hpp | 12 ++++++ rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp | 26 +++++++++++++ .../src/rmw_subscription.cpp | 23 +++++++++++ 7 files changed, 136 insertions(+), 2 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp index 1d101719d..fe93670e1 100644 --- a/rmw_fastrtps_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_cpp/src/rmw_publisher.cpp @@ -128,7 +128,8 @@ rmw_create_publisher( goto fail; } - info->publisher_ = Domain::createPublisher(participant, publisherParam, nullptr); + info->listener_ = new PubListener(info); + info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); @@ -181,6 +182,15 @@ rmw_create_publisher( return nullptr; } +rmw_ret_t +rmw_count_matched_subscriptions( + const rmw_publisher_t * publisher, + size_t * subscription_count) +{ + return rmw_fastrtps_shared_cpp::__rmw_count_matched_subscriptions( + publisher, subscription_count); +} + rmw_ret_t rmw_destroy_publisher(rmw_node_t * node, rmw_publisher_t * publisher) { diff --git a/rmw_fastrtps_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_cpp/src/rmw_subscription.cpp index 887d0ba22..2d1c5e159 100644 --- a/rmw_fastrtps_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_cpp/src/rmw_subscription.cpp @@ -164,6 +164,15 @@ rmw_create_subscription( return nullptr; } +rmw_ret_t +rmw_count_matched_publishers( + const rmw_subscription_t * subscription, + size_t * publisher_count) +{ + return rmw_fastrtps_shared_cpp::__rmw_count_matched_publishers( + subscription, publisher_count); +} + rmw_ret_t rmw_destroy_subscription(rmw_node_t * node, rmw_subscription_t * subscription) { diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp index 330977b9d..8bd87b5a3 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp @@ -15,18 +15,57 @@ #ifndef RMW_FASTRTPS_SHARED_CPP__CUSTOM_PUBLISHER_INFO_HPP_ #define RMW_FASTRTPS_SHARED_CPP__CUSTOM_PUBLISHER_INFO_HPP_ +#include +#include + #include "fastrtps/publisher/Publisher.h" +#include "fastrtps/publisher/PublisherListener.h" #include "rmw/rmw.h" #include "rmw_fastrtps_shared_cpp/TypeSupport.hpp" +class PubListener; + typedef struct CustomPublisherInfo { eprosima::fastrtps::Publisher * publisher_; + PubListener * listener_; rmw_fastrtps_shared_cpp::TypeSupport * type_support_; rmw_gid_t publisher_gid; const char * typesupport_identifier_; } CustomPublisherInfo; +class PubListener : public eprosima::fastrtps::PublisherListener +{ +public: + explicit PubListener(CustomPublisherInfo * info) + { + (void) info; + } + + void + onPublicationMatched( + eprosima::fastrtps::Publisher * pub, eprosima::fastrtps::rtps::MatchingInfo & info) + { + (void) pub; + std::lock_guard lock(internalMutex_); + if (info.status == eprosima::fastrtps::rtps::MATCHED_MATCHING) { + subscriptions_.insert(info.remoteEndpointGuid); + } else if (info.status == eprosima::fastrtps::rtps::REMOVED_MATCHING) { + subscriptions_.erase(info.remoteEndpointGuid); + } + } + + size_t subscriptionCount() + { + std::lock_guard lock(internalMutex_); + return subscriptions_.size(); + } + +private: + std::mutex internalMutex_; + std::set subscriptions_; +}; + #endif // RMW_FASTRTPS_SHARED_CPP__CUSTOM_PUBLISHER_INFO_HPP_ diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp index e597d6caf..4c8cfa412 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include "fastrtps/subscriber/Subscriber.h" @@ -51,7 +52,13 @@ class SubListener : public eprosima::fastrtps::SubscriberListener eprosima::fastrtps::Subscriber * sub, eprosima::fastrtps::rtps::MatchingInfo & info) { (void)sub; - (void)info; + + std::lock_guard lock(internalMutex_); + if (info.status == eprosima::fastrtps::rtps::MATCHED_MATCHING) { + publishers_.insert(info.remoteEndpointGuid); + } else if (info.status == eprosima::fastrtps::rtps::REMOVED_MATCHING) { + publishers_.erase(info.remoteEndpointGuid); + } } void @@ -107,11 +114,19 @@ class SubListener : public eprosima::fastrtps::SubscriberListener } } + size_t publisherCount() + { + std::lock_guard lock(internalMutex_); + return publishers_.size(); + } + private: std::mutex internalMutex_; std::atomic_size_t data_; std::mutex * conditionMutex_; std::condition_variable * conditionVariable_; + + std::set publishers_; }; #endif // RMW_FASTRTPS_SHARED_CPP__CUSTOM_SUBSCRIBER_INFO_HPP_ diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp index 44c06a77e..7163ba03b 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp @@ -129,6 +129,12 @@ __rmw_destroy_publisher( rmw_node_t * node, rmw_publisher_t * publisher); +RMW_FASTRTPS_SHARED_CPP_PUBLIC +rmw_ret_t +__rmw_count_matched_subscriptions( + const rmw_publisher_t * publisher, + size_t * subscription_count); + RMW_FASTRTPS_SHARED_CPP_PUBLIC rmw_ret_t __rmw_send_request( @@ -193,6 +199,12 @@ __rmw_destroy_subscription( rmw_node_t * node, rmw_subscription_t * subscription); +RMW_FASTRTPS_SHARED_CPP_PUBLIC +rmw_ret_t +__rmw_count_matched_publishers( + const rmw_subscription_t * subscription, + size_t * publisher_count); + RMW_FASTRTPS_SHARED_CPP_PUBLIC rmw_ret_t __rmw_take( diff --git a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp index 8458e6033..4088a75f4 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp @@ -64,6 +64,9 @@ __rmw_destroy_publisher( if (info->publisher_ != nullptr) { Domain::removePublisher(info->publisher_); } + if (info->listener_ != nullptr) { + delete info->listener_; + } if (info->type_support_ != nullptr) { auto impl = static_cast(node->data); if (!impl) { @@ -82,4 +85,27 @@ __rmw_destroy_publisher( return RMW_RET_OK; } + +rmw_ret_t +__rmw_count_matched_subscriptions( + const rmw_publisher_t * publisher, + size_t * subscription_count) +{ + if (!publisher) { + RMW_SET_ERROR_MSG("publisher handle is null"); + return RMW_RET_ERROR; + } + + if (!subscription_count) { + RMW_SET_ERROR_MSG("subscription_count is null"); + return RMW_RET_ERROR; + } + + auto info = static_cast(publisher->data); + if (info != nullptr) { + *subscription_count = info->listener_->subscriptionCount(); + } + + return RMW_RET_OK; +} } // namespace rmw_fastrtps_shared_cpp diff --git a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp index 1a52e8dec..4ad8f159d 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp @@ -91,4 +91,27 @@ __rmw_destroy_subscription( return RMW_RET_OK; } + +rmw_ret_t +__rmw_count_matched_publishers( + const rmw_subscription_t * subscription, + size_t * publisher_count) +{ + if (!subscription) { + RMW_SET_ERROR_MSG("subscription handle is null"); + return RMW_RET_ERROR; + } + + if (!publisher_count) { + RMW_SET_ERROR_MSG("publisher_count is null"); + return RMW_RET_ERROR; + } + + auto info = static_cast(subscription->data); + if (info != nullptr) { + *publisher_count = info->listener_->publisherCount(); + } + return RMW_RET_OK; +} + } // namespace rmw_fastrtps_shared_cpp From 75f48e3f477f825569d4f76dc63d08eaf1a8ef52 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 13 Nov 2018 11:28:35 -0600 Subject: [PATCH 2/7] Address reviewer feedback. --- rmw_fastrtps_cpp/src/rmw_publisher.cpp | 4 ++-- rmw_fastrtps_cpp/src/rmw_subscription.cpp | 4 ++-- rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp | 9 +++++++++ rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp | 9 +++++++++ .../include/rmw_fastrtps_shared_cpp/rmw_common.hpp | 4 ++-- rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp | 6 +++--- rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp | 6 +++--- 7 files changed, 30 insertions(+), 12 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp index fe93670e1..8da484fbb 100644 --- a/rmw_fastrtps_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_cpp/src/rmw_publisher.cpp @@ -183,11 +183,11 @@ rmw_create_publisher( } rmw_ret_t -rmw_count_matched_subscriptions( +rmw_publisher_count_matched_subscriptions( const rmw_publisher_t * publisher, size_t * subscription_count) { - return rmw_fastrtps_shared_cpp::__rmw_count_matched_subscriptions( + return rmw_fastrtps_shared_cpp::__rmw_publisher_count_matched_subscriptions( publisher, subscription_count); } diff --git a/rmw_fastrtps_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_cpp/src/rmw_subscription.cpp index 2d1c5e159..9f26ccc2e 100644 --- a/rmw_fastrtps_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_cpp/src/rmw_subscription.cpp @@ -165,11 +165,11 @@ rmw_create_subscription( } rmw_ret_t -rmw_count_matched_publishers( +rmw_subscription_count_matched_publishers( const rmw_subscription_t * subscription, size_t * publisher_count) { - return rmw_fastrtps_shared_cpp::__rmw_count_matched_publishers( + return rmw_fastrtps_shared_cpp::__rmw_subscription_count_matched_publishers( subscription, publisher_count); } diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp index 505d5f710..0c691361e 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp @@ -181,6 +181,15 @@ rmw_create_publisher( return nullptr; } +rmw_ret_t +rmw_publisher_count_matched_subscriptions( + const rmw_publisher_t * publisher, + size_t * subscription_count) +{ + return rmw_fastrtps_shared_cpp::__rmw_publisher_count_matched_subscriptions( + publisher, subscription_count); +} + rmw_ret_t rmw_destroy_publisher(rmw_node_t * node, rmw_publisher_t * publisher) { diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp index 6253a4598..0f9f81653 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp @@ -165,6 +165,15 @@ rmw_create_subscription( return nullptr; } +rmw_ret_t +rmw_subscription_count_matched_publishers( + const rmw_subscription_t * subscription, + size_t * publisher_count) +{ + return rmw_fastrtps_shared_cpp::__rmw_subscription_count_matched_publishers( + subscription, publisher_count); +} + rmw_ret_t rmw_destroy_subscription(rmw_node_t * node, rmw_subscription_t * subscription) { diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp index 7163ba03b..a4d968cb3 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/rmw_common.hpp @@ -131,7 +131,7 @@ __rmw_destroy_publisher( RMW_FASTRTPS_SHARED_CPP_PUBLIC rmw_ret_t -__rmw_count_matched_subscriptions( +__rmw_publisher_count_matched_subscriptions( const rmw_publisher_t * publisher, size_t * subscription_count); @@ -201,7 +201,7 @@ __rmw_destroy_subscription( RMW_FASTRTPS_SHARED_CPP_PUBLIC rmw_ret_t -__rmw_count_matched_publishers( +__rmw_subscription_count_matched_publishers( const rmw_subscription_t * subscription, size_t * publisher_count); diff --git a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp index 4088a75f4..e40561261 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp @@ -87,18 +87,18 @@ __rmw_destroy_publisher( } rmw_ret_t -__rmw_count_matched_subscriptions( +__rmw_publisher_count_matched_subscriptions( const rmw_publisher_t * publisher, size_t * subscription_count) { if (!publisher) { RMW_SET_ERROR_MSG("publisher handle is null"); - return RMW_RET_ERROR; + return RMW_RET_INVALID_ARGUMENT; } if (!subscription_count) { RMW_SET_ERROR_MSG("subscription_count is null"); - return RMW_RET_ERROR; + return RMW_RET_INVALID_ARGUMENT; } auto info = static_cast(publisher->data); diff --git a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp index 4ad8f159d..b4ea78ab2 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp @@ -93,18 +93,18 @@ __rmw_destroy_subscription( } rmw_ret_t -__rmw_count_matched_publishers( +__rmw_subscription_count_matched_publishers( const rmw_subscription_t * subscription, size_t * publisher_count) { if (!subscription) { RMW_SET_ERROR_MSG("subscription handle is null"); - return RMW_RET_ERROR; + return RMW_RET_INVALID_ARGUMENT; } if (!publisher_count) { RMW_SET_ERROR_MSG("publisher_count is null"); - return RMW_RET_ERROR; + return RMW_RET_INVALID_ARGUMENT; } auto info = static_cast(subscription->data); From f74b2b35f7dc6342139985abb5f46301197d9142 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 26 Nov 2018 23:27:50 -0600 Subject: [PATCH 3/7] Fix missing publisher. --- rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp | 11 ++++++++++- rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp | 9 +++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp index 0c691361e..f54e9baf4 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp @@ -128,7 +128,13 @@ rmw_create_publisher( goto fail; } - info->publisher_ = Domain::createPublisher(participant, publisherParam, nullptr); + info->listener_ = new PubListener(info); + if (!info->listener_) { + RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); + goto fail; + } + + info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); @@ -171,6 +177,9 @@ rmw_create_publisher( if (info->type_support_ != nullptr) { delete info->type_support_; } + if (info->listener_ != nullptr) { + delete info->listener_; + } delete info; } diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp index 0f9f81653..8d23351d1 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp @@ -124,6 +124,12 @@ rmw_create_subscription( } info->listener_ = new SubListener(info); + + if (!info->listener_) { + RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber listener"); + goto fail; + } + info->subscriber_ = Domain::createSubscriber(participant, subscriberParam, info->listener_); if (!info->subscriber_) { @@ -155,6 +161,9 @@ rmw_create_subscription( if (info->type_support_ != nullptr) { delete info->type_support_; } + if (info->listener_ != nullptr) { + delete info->listener_; + } delete info; } From 47b1a526ca76b3ffc0a1fd9c3076d189c537c86e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 27 Nov 2018 09:40:29 -0600 Subject: [PATCH 4/7] Fix some potential leaks. --- rmw_fastrtps_cpp/src/rmw_publisher.cpp | 9 ++++++++- rmw_fastrtps_cpp/src/rmw_subscription.cpp | 9 ++++++++- rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp | 1 - rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp | 2 -- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp index 8da484fbb..a7e488d8d 100644 --- a/rmw_fastrtps_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_cpp/src/rmw_publisher.cpp @@ -129,8 +129,12 @@ rmw_create_publisher( } info->listener_ = new PubListener(info); - info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); + if (!info->listener_) { + RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); + goto fail; + } + info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); goto fail; @@ -172,6 +176,9 @@ rmw_create_publisher( if (info->type_support_ != nullptr) { delete info->type_support_; } + if (info->listener_ != nullptr) { + delete info->listener_; + } delete info; } diff --git a/rmw_fastrtps_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_cpp/src/rmw_subscription.cpp index 9f26ccc2e..55fdf7780 100644 --- a/rmw_fastrtps_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_cpp/src/rmw_subscription.cpp @@ -123,8 +123,12 @@ rmw_create_subscription( } info->listener_ = new SubListener(info); - info->subscriber_ = Domain::createSubscriber(participant, subscriberParam, info->listener_); + if (!info->listener_) { + RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber listener"); + goto fail; + } + info->subscriber_ = Domain::createSubscriber(participant, subscriberParam, info->listener_); if (!info->subscriber_) { RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber"); goto fail; @@ -154,6 +158,9 @@ rmw_create_subscription( if (info->type_support_ != nullptr) { delete info->type_support_; } + if (info->listener_ != nullptr) { + delete info->listener_; + } delete info; } diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp index f54e9baf4..4cf58c0de 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp @@ -135,7 +135,6 @@ rmw_create_publisher( } info->publisher_ = Domain::createPublisher(participant, publisherParam, info->listener_); - if (!info->publisher_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher"); goto fail; diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp index 8d23351d1..21db6117d 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp @@ -124,14 +124,12 @@ rmw_create_subscription( } info->listener_ = new SubListener(info); - if (!info->listener_) { RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber listener"); goto fail; } info->subscriber_ = Domain::createSubscriber(participant, subscriberParam, info->listener_); - if (!info->subscriber_) { RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber"); goto fail; From 38575cb90f47874965b01a7e1f894bb92e4e8eb9 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 27 Nov 2018 09:44:13 -0600 Subject: [PATCH 5/7] Replace with CHECK_ARGUMENT_FOR_NULL --- rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp | 11 ++--------- rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp | 11 ++--------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp index e40561261..f31301e4d 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_publisher.cpp @@ -91,15 +91,8 @@ __rmw_publisher_count_matched_subscriptions( const rmw_publisher_t * publisher, size_t * subscription_count) { - if (!publisher) { - RMW_SET_ERROR_MSG("publisher handle is null"); - return RMW_RET_INVALID_ARGUMENT; - } - - if (!subscription_count) { - RMW_SET_ERROR_MSG("subscription_count is null"); - return RMW_RET_INVALID_ARGUMENT; - } + RMW_CHECK_ARGUMENT_FOR_NULL(publisher, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_ARGUMENT_FOR_NULL(subscription_count, RMW_RET_INVALID_ARGUMENT); auto info = static_cast(publisher->data); if (info != nullptr) { diff --git a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp index b4ea78ab2..c237aa5cf 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp @@ -97,15 +97,8 @@ __rmw_subscription_count_matched_publishers( const rmw_subscription_t * subscription, size_t * publisher_count) { - if (!subscription) { - RMW_SET_ERROR_MSG("subscription handle is null"); - return RMW_RET_INVALID_ARGUMENT; - } - - if (!publisher_count) { - RMW_SET_ERROR_MSG("publisher_count is null"); - return RMW_RET_INVALID_ARGUMENT; - } + RMW_CHECK_ARGUMENT_FOR_NULL(subscription, RMW_RET_INVALID_ARGUMENT); + RMW_CHECK_ARGUMENT_FOR_NULL(publisher_count, RMW_RET_INVALID_ARGUMENT); auto info = static_cast(subscription->data); if (info != nullptr) { From 311275a888327fa3fe2bb4961a07afe25f76bef8 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 27 Nov 2018 09:49:55 -0600 Subject: [PATCH 6/7] Apply suggestions from code review Co-Authored-By: mjcarroll --- .../include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp | 4 ++-- .../rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp index 8bd87b5a3..486f10869 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp @@ -50,9 +50,9 @@ class PubListener : public eprosima::fastrtps::PublisherListener { (void) pub; std::lock_guard lock(internalMutex_); - if (info.status == eprosima::fastrtps::rtps::MATCHED_MATCHING) { + if (eprosima::fastrtps::rtps::MATCHED_MATCHING == info.status) { subscriptions_.insert(info.remoteEndpointGuid); - } else if (info.status == eprosima::fastrtps::rtps::REMOVED_MATCHING) { + } else if (eprosima::fastrtps::rtps::REMOVED_MATCHING == info.status) { subscriptions_.erase(info.remoteEndpointGuid); } } diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp index 4c8cfa412..f3938a146 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp @@ -54,9 +54,9 @@ class SubListener : public eprosima::fastrtps::SubscriberListener (void)sub; std::lock_guard lock(internalMutex_); - if (info.status == eprosima::fastrtps::rtps::MATCHED_MATCHING) { + if (eprosima::fastrtps::rtps::MATCHED_MATCHING == info.status) { publishers_.insert(info.remoteEndpointGuid); - } else if (info.status == eprosima::fastrtps::rtps::REMOVED_MATCHING) { + } else if (eprosima::fastrtps::rtps::REMOVED_MATCHING == info.status) { publishers_.erase(info.remoteEndpointGuid); } } From 6074f18cb8f2431b7562ac1849928006e6656a09 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 27 Nov 2018 15:35:12 -0600 Subject: [PATCH 7/7] Address reviewer feedback. --- rmw_fastrtps_cpp/src/rmw_publisher.cpp | 15 ++++++++++++--- rmw_fastrtps_cpp/src/rmw_subscription.cpp | 15 ++++++++++++--- rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp | 8 ++++++-- rmw_fastrtps_dynamic_cpp/src/rmw_serialize.cpp | 3 ++- rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp | 8 ++++++-- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_cpp/src/rmw_publisher.cpp index a7e488d8d..5907590fd 100644 --- a/rmw_fastrtps_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_cpp/src/rmw_publisher.cpp @@ -92,7 +92,12 @@ rmw_create_publisher( Domain::getDefaultPublisherAttributes(publisherParam); // TODO(karsten1987) Verify consequences for std::unique_ptr? - info = new CustomPublisherInfo(); + info = new (std::nothrow) CustomPublisherInfo(); + if (!info) { + RMW_SET_ERROR_MSG("failed to allocate CustomPublisherInfo"); + return nullptr; + } + info->typesupport_identifier_ = type_support->typesupport_identifier; auto callbacks = static_cast(type_support->data); @@ -100,7 +105,11 @@ rmw_create_publisher( if (!Domain::getRegisteredType(participant, type_name.c_str(), reinterpret_cast(&info->type_support_))) { - info->type_support_ = new MessageTypeSupport_cpp(callbacks); + info->type_support_ = new (std::nothrow) MessageTypeSupport_cpp(callbacks); + if (!info->type_support_) { + RMW_SET_ERROR_MSG("Failed to allocate MessageTypeSupport"); + goto fail; + } _register_type(participant, info->type_support_); } @@ -128,7 +137,7 @@ rmw_create_publisher( goto fail; } - info->listener_ = new PubListener(info); + info->listener_ = new (std::nothrow) PubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); goto fail; diff --git a/rmw_fastrtps_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_cpp/src/rmw_subscription.cpp index 55fdf7780..998b859f8 100644 --- a/rmw_fastrtps_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_cpp/src/rmw_subscription.cpp @@ -95,7 +95,12 @@ rmw_create_subscription( // Load default XML profile. Domain::getDefaultSubscriberAttributes(subscriberParam); - info = new CustomSubscriberInfo(); + info = new (std::nothrow) CustomSubscriberInfo(); + if (!info) { + RMW_SET_ERROR_MSG("failed to allocate CustomSubscriberInfo"); + return nullptr; + } + info->typesupport_identifier_ = type_support->typesupport_identifier; auto callbacks = static_cast(type_support->data); @@ -103,7 +108,11 @@ rmw_create_subscription( if (!Domain::getRegisteredType(participant, type_name.c_str(), reinterpret_cast(&info->type_support_))) { - info->type_support_ = new MessageTypeSupport_cpp(callbacks); + info->type_support_ = new (std::nothrow) MessageTypeSupport_cpp(callbacks); + if (!info->type_support_) { + RMW_SET_ERROR_MSG("failed to allocate MessageTypeSupport_cpp"); + goto fail; + } _register_type(participant, info->type_support_); } @@ -122,7 +131,7 @@ rmw_create_subscription( goto fail; } - info->listener_ = new SubListener(info); + info->listener_ = new (std::nothrow) SubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber listener"); goto fail; diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp index 4cf58c0de..ae44cfe03 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_publisher.cpp @@ -91,7 +91,11 @@ rmw_create_publisher( Domain::getDefaultPublisherAttributes(publisherParam); // TODO(karsten1987) Verify consequences for std::unique_ptr? - info = new CustomPublisherInfo(); + info = new (std::nothrow) CustomPublisherInfo(); + if (!info) { + RMW_SET_ERROR_MSG("failed to allocate CustomPublisherInfo"); + return nullptr; + } info->typesupport_identifier_ = type_support->typesupport_identifier; std::string type_name = _create_type_name( @@ -128,7 +132,7 @@ rmw_create_publisher( goto fail; } - info->listener_ = new PubListener(info); + info->listener_ = new (std::nothrow) PubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_publisher() could not create publisher listener"); goto fail; diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_serialize.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_serialize.cpp index 6af64af4f..64efe0910 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_serialize.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_serialize.cpp @@ -48,7 +48,8 @@ rmw_serialize( } } - eprosima::fastcdr::FastBuffer buffer(reinterpret_cast(serialized_message->buffer), + eprosima::fastcdr::FastBuffer buffer( + reinterpret_cast(serialized_message->buffer), data_length); eprosima::fastcdr::Cdr ser( buffer, eprosima::fastcdr::Cdr::DEFAULT_ENDIAN, eprosima::fastcdr::Cdr::DDS_CDR); diff --git a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp index 21db6117d..a421424fe 100644 --- a/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_dynamic_cpp/src/rmw_subscription.cpp @@ -95,7 +95,11 @@ rmw_create_subscription( // Load default XML profile. Domain::getDefaultSubscriberAttributes(subscriberParam); - info = new CustomSubscriberInfo(); + info = new (std::nothrow) CustomSubscriberInfo(); + if (!info) { + RMW_SET_ERROR_MSG("failed to allocate CustomSubscriberInfo"); + return nullptr; + } info->typesupport_identifier_ = type_support->typesupport_identifier; std::string type_name = _create_type_name( @@ -123,7 +127,7 @@ rmw_create_subscription( goto fail; } - info->listener_ = new SubListener(info); + info->listener_ = new (std::nothrow) SubListener(info); if (!info->listener_) { RMW_SET_ERROR_MSG("create_subscriber() could not create subscriber listener"); goto fail;