From c14badd61bb52deace166efbe06afa1c4721670d Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Mon, 10 Jun 2024 11:54:43 +0200 Subject: [PATCH] Refs #21096: Applied Edus review Signed-off-by: Mario Dominguez --- include/fastdds/dds/core/Types.hpp | 2 +- .../dds/core/policy/ParameterTypes.hpp | 9 +++--- .../core/policy/ParameterSerializer.hpp | 9 ++---- .../builtin/data/ParticipantProxyData.cpp | 6 ++-- .../participant/PDPClientListener.hpp | 2 -- .../discovery/participant/PDPListener.cpp | 6 ++-- .../participant/PDPServerListener.cpp | 32 +++++++++---------- .../participant/PDPServerListener.hpp | 13 ++++++-- .../rtps/participant/RTPSParticipantImpl.cpp | 6 ++++ .../common/DDSBlackboxTestsDiscovery.cpp | 2 -- ...ery_participants_initial_peers_profile.xml | 6 ++-- 11 files changed, 49 insertions(+), 44 deletions(-) diff --git a/include/fastdds/dds/core/Types.hpp b/include/fastdds/dds/core/Types.hpp index 2142681298e..667616d0a25 100644 --- a/include/fastdds/dds/core/Types.hpp +++ b/include/fastdds/dds/core/Types.hpp @@ -23,7 +23,7 @@ namespace dds { typedef uint32_t DomainId_t; -const DomainId_t c_DomainId_t_Unknown = 0xFFFFFFFF; +const DomainId_t DOMAIN_ID_UNKNOWN = 0xFFFFFFFF; const int32_t LENGTH_UNLIMITED = -1; diff --git a/include/fastdds/dds/core/policy/ParameterTypes.hpp b/include/fastdds/dds/core/policy/ParameterTypes.hpp index f79ba81a923..c09da61d6d1 100644 --- a/include/fastdds/dds/core/policy/ParameterTypes.hpp +++ b/include/fastdds/dds/core/policy/ParameterTypes.hpp @@ -27,6 +27,7 @@ #include +#include #include #include #include @@ -532,14 +533,14 @@ class ParameterDomainId_t : public Parameter_t { public: - //!Domain ID.
By default, 0. + //!Domain ID.
By default, DOMAIN_ID_UNKNOWN. uint32_t domain_id; /** * @brief Constructor without parameters */ ParameterDomainId_t() - : domain_id(0) + : domain_id(DOMAIN_ID_UNKNOWN) { } @@ -553,9 +554,9 @@ class ParameterDomainId_t : public Parameter_t ParameterId_t pid, uint16_t in_length) : Parameter_t(pid, in_length) - , domain_id(0) + , domain_id(DOMAIN_ID_UNKNOWN) { - domain_id = 0; + domain_id = DOMAIN_ID_UNKNOWN; } }; diff --git a/src/cpp/fastdds/core/policy/ParameterSerializer.hpp b/src/cpp/fastdds/core/policy/ParameterSerializer.hpp index a7dca57e105..6d9cff6e72a 100644 --- a/src/cpp/fastdds/core/policy/ParameterSerializer.hpp +++ b/src/cpp/fastdds/core/policy/ParameterSerializer.hpp @@ -438,8 +438,7 @@ inline bool ParameterSerializer::add_content_to_cdr_message const ParameterDomainId_t& parameter, fastrtps::rtps::CDRMessage_t* cdr_message) { - bool valid = fastrtps::rtps::CDRMessage::addUInt32(cdr_message, parameter.domain_id); - return valid; + return fastrtps::rtps::CDRMessage::addUInt32(cdr_message, parameter.domain_id); } template<> @@ -448,14 +447,12 @@ inline bool ParameterSerializer::read_content_from_cdr_mess fastrtps::rtps::CDRMessage_t* cdr_message, const uint16_t parameter_length) { - if (parameter_length != PARAMETER_VENDOR_LENGTH) + if (parameter_length != PARAMETER_DOMAINID_LENGTH) { return false; } parameter.length = parameter_length; - bool valid = fastrtps::rtps::CDRMessage::readUInt32(cdr_message, ¶meter.domain_id); - cdr_message->pos += 2; //padding - return valid; + return fastrtps::rtps::CDRMessage::readUInt32(cdr_message, ¶meter.domain_id); } template<> diff --git a/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp b/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp index f135c4e3847..a78cb3c2181 100644 --- a/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp +++ b/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp @@ -52,7 +52,7 @@ ParticipantProxyData::ParticipantProxyData( const RTPSParticipantAllocationAttributes& allocation) : m_protocolVersion(c_ProtocolVersion) , m_VendorId(c_VendorId_Unknown) - , m_domain_id(fastdds::dds::c_DomainId_t_Unknown) + , m_domain_id(fastdds::dds::DOMAIN_ID_UNKNOWN) , m_expectsInlineQos(false) , m_availableBuiltinEndpoints(0) , m_networkConfiguration(0) @@ -154,7 +154,7 @@ uint32_t ParticipantProxyData::get_serialized_size( ret_val += 4 + 4; // PID_DOMAIN_ID - ret_val += 4 + 4; + ret_val += 4 + PARAMETER_DOMAINID_LENGTH; if (m_expectsInlineQos) { @@ -765,7 +765,7 @@ void ParticipantProxyData::clear() m_guid = GUID_t(); //set_VendorId_Unknown(m_VendorId); m_VendorId = c_VendorId_Unknown; - m_domain_id = fastdds::dds::c_DomainId_t_Unknown; + m_domain_id = fastdds::dds::DOMAIN_ID_UNKNOWN; m_expectsInlineQos = false; m_availableBuiltinEndpoints = 0; m_networkConfiguration = 0; diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPClientListener.hpp b/src/cpp/rtps/builtin/discovery/participant/PDPClientListener.hpp index 3e645d9e9f7..3d76e7d6d16 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPClientListener.hpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPClientListener.hpp @@ -19,7 +19,6 @@ #ifndef _FASTDDS_RTPS_PDPCLIENTLISTENER_H_ #define _FASTDDS_RTPS_PDPCLIENTLISTENER_H_ -#ifndef DOXYGEN_SHOULD_SKIP_THIS_PUBLIC #include @@ -58,5 +57,4 @@ class PDPClientListener : public PDPListener } /* namespace fastrtps */ } /* namespace eprosima */ -#endif // ifndef DOXYGEN_SHOULD_SKIP_THIS_PUBLIC #endif /* _FASTDDS_RTPS_PDPCLIENTLISTENER_H_ */ diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp index 1e86bf52b28..a6ae4060c89 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp @@ -114,9 +114,7 @@ void PDPListener::onNewCacheChangeAdded( return; } - void* empty_extra_data = nullptr; - static_cast(empty_extra_data); - if (!check_discovery_conditions(temp_participant_data_, empty_extra_data)) + if (!check_discovery_conditions(temp_participant_data_, nullptr)) { return; } @@ -291,7 +289,7 @@ bool PDPListener::check_discovery_conditions( // In PDPSimple, do not match if the participant is from a different domain. // If the domain id is unknown, it is assumed to be the same domain if (remote_participant_domain_id != parent_pdp_->getRTPSParticipant()->get_domain_id() && - remote_participant_domain_id != fastdds::dds::c_DomainId_t_Unknown) + remote_participant_domain_id != fastdds::dds::DOMAIN_ID_UNKNOWN) { EPROSIMA_LOG_INFO(RTPS_PDP_DISCOVERY, "Received participant with different domain id (" << remote_participant_domain_id << ") than ours (" diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp index ebc7676fc64..6d55dddc0a6 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp @@ -148,11 +148,12 @@ void PDPServerListener::onNewCacheChangeAdded( return; } - bool is_client = true; - if (!check_discovery_conditions(participant_data, &is_client)) + auto ret = check_server_discovery_conditions(participant_data); + if (!ret.first) { return; } + bool is_client = ret.second; const auto& pattr = pdp_server()->getRTPSParticipant()->getAttributes(); fastdds::rtps::network::external_locators::filter_remote_locators(participant_data, @@ -361,18 +362,18 @@ void PDPServerListener::onNewCacheChangeAdded( EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, ""); } -bool PDPServerListener::check_discovery_conditions( - ParticipantProxyData& participant_data, - void* extra_data /* bool is_client*/) +std::pair PDPServerListener::check_server_discovery_conditions( + ParticipantProxyData& participant_data) { - bool ret = true; + // is_valid, is_client + std::pair ret{true, true}; /* Check PID_VENDOR_ID */ if (participant_data.m_VendorId != fastrtps::rtps::c_VendorId_eProsima) { EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, "DATA(p|Up) from different vendor is not supported for Discover-Server operation"); - ret = false; + ret.first = false; } // In Discovery Server we don't impose @@ -382,7 +383,7 @@ bool PDPServerListener::check_discovery_conditions( fastdds::dds::ParameterPropertyList_t properties = participant_data.m_properties; /* Check DS_VERSION */ - if (ret) + if (ret.first) { auto ds_version = std::find_if( properties.begin(), @@ -398,7 +399,7 @@ bool PDPServerListener::check_discovery_conditions( { EPROSIMA_LOG_ERROR(RTPS_PDP_LISTENER, "Minimum " << dds::parameter_property_ds_version << " is 1.0, found: " << ds_version->second()); - ret = false; + ret.first = false; } EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, "Participant " << dds::parameter_property_ds_version << ": " << ds_version->second()); @@ -410,7 +411,7 @@ bool PDPServerListener::check_discovery_conditions( } /* Check PARTICIPANT_TYPE */ - if (ret) + if (ret.first) { auto participant_type = std::find_if( properties.begin(), @@ -426,19 +427,19 @@ bool PDPServerListener::check_discovery_conditions( participant_type->second() == ParticipantType::BACKUP || participant_type->second() == ParticipantType::SUPER_CLIENT) { - *static_cast(extra_data) = false; + ret.second = false; } else if (participant_type->second() == ParticipantType::SIMPLE) { EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, "Ignoring " << dds::parameter_property_participant_type << ": " << participant_type->second()); - ret = false; + ret.first = false; } else if (participant_type->second() != ParticipantType::CLIENT) { EPROSIMA_LOG_ERROR(RTPS_PDP_LISTENER, "Wrong " << dds::parameter_property_participant_type << ": " << participant_type->second()); - ret = false; + ret.first = false; } EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, "Participant type " << participant_type->second()); } @@ -458,11 +459,10 @@ bool PDPServerListener::check_discovery_conditions( // as persistent will have this property. if (persistence_guid != properties.end()) { - // is_client = false - *static_cast(extra_data) = false; + ret.second = false; } EPROSIMA_LOG_INFO(RTPS_PDP_LISTENER, - "Participant is client: " << std::boolalpha << *static_cast(extra_data)); + "Participant is client: " << std::boolalpha << ret.second); } } diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.hpp b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.hpp index 8778ffffb30..252b732f2c7 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.hpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.hpp @@ -61,9 +61,16 @@ class PDPServerListener : public fastrtps::rtps::PDPListener protected: - bool check_discovery_conditions( - fastrtps::rtps::ParticipantProxyData& participant_data, - void* extra_data) override; + /** + * Checks discovery conditions on a discovery server entity. + * Essentially, it checks for incoming PIDS of remote proxy datas. + * @param participant_data Remote participant data to check. + * @return A pair of booleans. + * The first one indicates if the remote participant data is valid. + * The second one indicates if the remote participant data is a client. + */ + std::pair check_server_discovery_conditions( + fastrtps::rtps::ParticipantProxyData& participant_data); }; diff --git a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp index fafc4765233..c391d3a166b 100644 --- a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp +++ b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp @@ -277,6 +277,12 @@ RTPSParticipantImpl::RTPSParticipantImpl( , has_shm_transport_(false) , match_local_endpoints_(should_match_local_endpoints(PParam)) { + if (domain_id_ == fastdds::dds::DOMAIN_ID_UNKNOWN) + { + EPROSIMA_LOG_ERROR(RTPS_PARTICIPANT, "Domain ID has to be set to a correct value"); + return; + } + if (c_GuidPrefix_Unknown != persistence_guid) { m_persistence_guid = GUID_t(persistence_guid, c_EntityId_RTPSParticipant); diff --git a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp index 4f28859b140..d5608070db2 100644 --- a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp @@ -2034,7 +2034,6 @@ TEST(DDSDiscovery, pdp_simple_participants_exchange_same_pid_domain_id_and_disco * do not discover each other despite the first one is initial peer of the second. * */ - TEST(DDSDiscovery, pdp_simple_initial_peer_participants_with_different_domain_ids_do_not_discover) { PubSubWriter writer_domain_1(TEST_TOPIC_NAME); @@ -2064,7 +2063,6 @@ TEST(DDSDiscovery, pdp_simple_initial_peer_participants_with_different_domain_id * discover each other. * */ - TEST(DDSDiscovery, client_server_participants_with_different_domain_ids_discover) { PubSubReader server_domain_1(TEST_TOPIC_NAME); diff --git a/test/blackbox/discovery_participants_initial_peers_profile.xml b/test/blackbox/discovery_participants_initial_peers_profile.xml index e87846c742e..fe17378473d 100644 --- a/test/blackbox/discovery_participants_initial_peers_profile.xml +++ b/test/blackbox/discovery_participants_initial_peers_profile.xml @@ -25,7 +25,7 @@
127.0.0.1
- 7412 + 11500 @@ -50,14 +50,14 @@
127.0.0.1
- 7412 + 11500 - 7400 + 11600
239.255.0.1