Skip to content

Commit 22743c7

Browse files
cferreiragonzEugenioCollado
cferreiragonz
authored and
EugenioCollado
committed
Avoid calling setIPv4 in TCPTransportInterface (#5492)
* Refs #22469: Regression Tests for IPv6 Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: New method copyAddress Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Fix TCPTransportInterface Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Apply Review Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Disable grouping of locators in single channel for TCPv6 Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Uncrustify Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Apply review Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Allow to copyIPv4 between two locators Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #22469: Apply Review 2 Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>
1 parent 898ee97 commit 22743c7

6 files changed

Lines changed: 110 additions & 15 deletions

File tree

‎include/fastrtps/utils/IPLocator.h‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,27 @@ class IPLocator
9797
RTPS_DllAPI static std::string toIPv4string(
9898
const Locator_t& locator);
9999

100-
//! Copies locator's IPv4.
100+
/**
101+
* @brief Copies locator's IPv4 to a destination array.
102+
* @param locator Locator from which to copy the IPv4.
103+
* @param dest Destination array where the IPv4 will be copied.
104+
* @return true if the copy was successful, false otherwise.
105+
*/
101106
RTPS_DllAPI static bool copyIPv4(
102107
const Locator_t& locator,
103108
unsigned char* dest);
104109

110+
/**
111+
* @brief Copies locator's IPv4 to a destination locator.
112+
* It only copies the IPv4 part (last 4 bytes), leaving other parts unchanged.
113+
* @param locator Locator from which to copy the IPv4.
114+
* @param dest Destination locator where the IPv4 will be copied.
115+
* @return true if the copy was successful, false otherwise.
116+
*/
117+
RTPS_DllAPI static bool copyIPv4(
118+
const Locator_t& locator,
119+
Locator_t& dest);
120+
105121
// IPv6
106122
//! Sets locator's IPv6.
107123
RTPS_DllAPI static bool setIPv6(
@@ -252,6 +268,16 @@ class IPLocator
252268
const Locator_t& loc2,
253269
bool fullAddress = false);
254270

271+
/**
272+
* Copies the whole address from one locator to another.
273+
* @param loc1 Locator to copy from.
274+
* @param loc2 Locator to copy to.
275+
* @return True if the copy was successful.
276+
*/
277+
RTPS_DllAPI static bool copy_address(
278+
const Locator_t& loc1,
279+
Locator_t& loc2);
280+
255281
//! Checks if a both locators has the same IP address and physical port (as in RTCP protocol).
256282
RTPS_DllAPI static bool compareAddressAndPhysicalPort(
257283
const Locator_t& loc1,

‎src/cpp/rtps/transport/TCPTransportInterface.cpp‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -324,14 +324,18 @@ ResponseCode TCPTransportInterface::bind_socket(
324324

325325
std::vector<fastrtps::rtps::IPFinder::info_IP> local_interfaces;
326326
// Check if the locator is from an owned interface to link all local interfaces to the channel
327-
is_own_interface(channel->locator(), local_interfaces);
328-
if (!local_interfaces.empty())
327+
// Note: Only applicable for TCPv4 until TCPv6 scope selection is implemented
328+
if (channel->locator().kind != LOCATOR_KIND_TCPv6)
329329
{
330-
Locator local_locator(channel->locator());
331-
for (auto& interface_it : local_interfaces)
330+
is_own_interface(channel->locator(), local_interfaces);
331+
if (!local_interfaces.empty())
332332
{
333-
IPLocator::setIPv4(local_locator, interface_it.locator);
334-
channel_resources_.insert(decltype(channel_resources_)::value_type{local_locator, channel});
333+
Locator local_locator(channel->locator());
334+
for (auto& interface_it : local_interfaces)
335+
{
336+
IPLocator::copy_address(interface_it.locator, local_locator);
337+
channel_resources_.insert(decltype(channel_resources_)::value_type{local_locator, channel});
338+
}
335339
}
336340
}
337341
return ret;
@@ -1031,14 +1035,18 @@ bool TCPTransportInterface::CreateInitialConnect(
10311035

10321036
std::vector<fastrtps::rtps::IPFinder::info_IP> local_interfaces;
10331037
// Check if the locator is from an owned interface to link all local interfaces to the channel
1034-
is_own_interface(physical_locator, local_interfaces);
1035-
if (!local_interfaces.empty())
1038+
// Note: Only applicable for TCPv4 until TCPv6 scope selection is implemented
1039+
if (physical_locator.kind != LOCATOR_KIND_TCPv6)
10361040
{
1037-
Locator local_locator(physical_locator);
1038-
for (auto& interface_it : local_interfaces)
1041+
is_own_interface(physical_locator, local_interfaces);
1042+
if (!local_interfaces.empty())
10391043
{
1040-
IPLocator::setIPv4(local_locator, interface_it.locator);
1041-
channel_resources_[local_locator] = channel;
1044+
Locator local_locator(physical_locator);
1045+
for (auto& interface_it : local_interfaces)
1046+
{
1047+
IPLocator::copy_address(interface_it.locator, local_locator);
1048+
channel_resources_[local_locator] = channel;
1049+
}
10421050
}
10431051
}
10441052

@@ -1345,7 +1353,8 @@ bool TCPTransportInterface::Receive(
13451353
do
13461354
{
13471355
header_found = receive_header(channel, tcp_header, ec);
1348-
} while (!header_found && !ec && channel->connection_status());
1356+
}
1357+
while (!header_found && !ec && channel->connection_status());
13491358

13501359
if (ec)
13511360
{

‎src/cpp/utils/IPLocator.cpp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,13 @@ bool IPLocator::copyIPv4(
178178
return true;
179179
}
180180

181+
bool IPLocator::copyIPv4(
182+
const Locator_t& locator,
183+
Locator_t& dest)
184+
{
185+
return copyIPv4(locator, &(dest.address[12]));
186+
}
187+
181188
// IPv6
182189
bool IPLocator::setIPv6(
183190
Locator_t& locator,
@@ -911,6 +918,27 @@ bool IPLocator::compareAddress(
911918
}
912919
}
913920

921+
bool IPLocator::copy_address(
922+
const Locator_t& loc1,
923+
Locator_t& loc2)
924+
{
925+
if (loc1.kind != loc2.kind)
926+
{
927+
return false;
928+
}
929+
930+
if (loc1.kind == LOCATOR_KIND_UDPv4 || loc1.kind == LOCATOR_KIND_TCPv4)
931+
{
932+
copyIPv4(loc1, loc2);
933+
return true;
934+
}
935+
else if (loc1.kind == LOCATOR_KIND_UDPv6 || loc1.kind == LOCATOR_KIND_TCPv6)
936+
{
937+
return copyIPv6(loc1, loc2.address);
938+
}
939+
return false;
940+
}
941+
914942
bool IPLocator::compareAddressAndPhysicalPort(
915943
const Locator_t& loc1,
916944
const Locator_t& loc2)

‎test/unittest/transport/TCPv4Tests.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1955,6 +1955,7 @@ TEST_F(TCPv4Tests, client_announced_local_port_uniqueness)
19551955

19561956
std::this_thread::sleep_for(std::chrono::milliseconds(100));
19571957

1958+
EXPECT_GT(receiveTransportUnderTest.get_channel_resources().size(), 2u);
19581959
std::set<std::shared_ptr<TCPChannelResource>> channels_created;
19591960
for (const auto& channel_resource : receiveTransportUnderTest.get_channel_resources())
19601961
{

‎test/unittest/transport/TCPv6Tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ TEST_F(TCPv6Tests, client_announced_local_port_uniqueness)
367367

368368
std::this_thread::sleep_for(std::chrono::milliseconds(100));
369369

370-
ASSERT_EQ(receiveTransportUnderTest.get_channel_resources().size(), 2u);
370+
EXPECT_EQ(receiveTransportUnderTest.get_channel_resources().size(), 2u);
371371
}
372372

373373
#ifndef _WIN32

‎test/unittest/utils/LocatorTests.cpp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,6 +766,37 @@ TEST_F(IPLocatorTests, copyIPv6)
766766
ASSERT_EQ(arr[15], 1u);
767767
}
768768

769+
/*
770+
* Check to copy an address
771+
*/
772+
TEST_F(IPLocatorTests, copy_address)
773+
{
774+
// Copy IPv4
775+
Locator_t locator1(LOCATOR_KIND_UDPv4);
776+
Locator_t locator2(LOCATOR_KIND_UDPv4);
777+
IPLocator::setIPv4(locator1, ipv4_lo_address);
778+
ASSERT_FALSE(IPLocator::compareAddress(locator1, locator2));
779+
ASSERT_TRUE(IPLocator::copy_address(locator1, locator2));
780+
ASSERT_TRUE(IPLocator::compareAddress(locator1, locator2));
781+
782+
// Check cannot copy between different kinds
783+
locator1.kind = LOCATOR_KIND_UDPv6;
784+
ASSERT_FALSE(IPLocator::copy_address(locator1, locator2));
785+
786+
// Copy IPv6
787+
locator2.kind = LOCATOR_KIND_UDPv6;
788+
IPLocator::setIPv6(locator1, ipv6_lo_address);
789+
ASSERT_FALSE(IPLocator::compareAddress(locator1, locator2));
790+
ASSERT_TRUE(IPLocator::copy_address(locator1, locator2));
791+
ASSERT_TRUE(IPLocator::compareAddress(locator1, locator2));
792+
793+
// Check cannot copy between SHM locators
794+
locator1.kind = LOCATOR_KIND_SHM;
795+
Locator_t locator3(LOCATOR_KIND_SHM);
796+
ASSERT_FALSE(IPLocator::copy_address(locator1, locator3));
797+
ASSERT_FALSE(IPLocator::compareAddress(locator1, locator3));
798+
}
799+
769800
/*
770801
* Check to set ip of any kind
771802
*/

0 commit comments

Comments
 (0)