Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22984 )
Change subject: KUDU-1457 [1/n] add IPv6 support to net utils ...................................................................... Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/22984/12/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/12/src/kudu/rpc/rpc-test.cc@2084 PS12, Line 2084: constexpr size_t kClientsNum = 8; : : DiagnosticSocket::TcpSocketInfo info; : ASSERT_OK(RunAndGetSocketInfo(kListenBacklog, kClientsNum, &info)); : : ASSERT_EQ(kClientsNum, info.rx_queue_size); : ASSERT_EQ(kListenBacklog, info.tx_queue_size); : } : : // This scenario is similar to the couple of scenarios above, but it's not : // enough space in the socket's RX queue to accommodate all the pending TCP : // connections. : TEST_P(TestRpcSocketTxRxQueue, OverCapacity) { : constexpr int kListenBacklog = 5; : constexpr size_t kClientsN > nit: is it possible to consolidate this and another functor that's used at Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.h@95 PS11, Line 95: const std::string_view& host > As of PS12, it's still passed by const reference, not by value. Am I missi That's a miss from my side. Sorry about that! I have updated this to value now. You should be able see it in latest PS. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc@225 PS1, Line 225: } : if (storage_.un.sun_path[0] == > Indeed, the sin6_flowinfo come before the sin6_addr field in the sockaddr_i When you say "from outside" what does that mean? Is sockaddr_in6 coming from outside of Kudu library? If it is within Kudu library, we should not get any sockaddr_in6 address with non-zero fields (sin6_flowinfo and sin6_scope_id). If you think there is any possibility, can you please point that out from code. I want to make sure such scenario is covered from the test as well as functionality perspective and corresponding zeroing out of those fields is done at right place. On a side note, I don't mind zeroing out of these fields but I think that assignment operator method should just copy the address. It should not zero out or validate any field inside. The only check it should be doing is that correct copy ctor is called for the incoming IP address family type. In future, if we do introduce features like QoS, etc. the assignment operator method doesn't need to change anything. It can simply copy the field values as is from source to target object. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.cc@243 PS11, Line 243: Sockaddr& Sockaddr::operator=(const struct sockaddr_in6& addr) { : set_length(sizeof(addr)); : memcpy(&storage_, &addr, len_); : DCHECK_EQ(family(), AF_INET6); : return *this; : } > The point here is not exactly about the presence of garbage/non-initialized I didn't fully go through the RCA of KUDU-3352 but my high level understanding is that due to uninitialised sin_zero, FindOrStartConnection() could not find the existing connection with already established credentials so it went on to create a new connection to the remote target. So, fix rules out such possibility by ignoring sin_zero while doing comparison or finding hash. As far as these two fields are concerned, my understanding is that sin6_flowinfo can be set by applications and sin6_scope_id by OS network stack based on the requirement of QoS or determining the scope of link-local addresses. sin6_flowinfo cannot be non-zero unless our socket implementation allows it to do so. Since we are not doing any QoS handling or any other optimisation that can potentially make use of this field, I would be surprised if it has a non-zero value. sin6_scope_id is something that is set by OS to identify the interface that an IPv6 address (link-local) is associated with. So, there is a possibility of this being non-zero when there are multiple interfaces with same link-local addresses. Even with that, if say GetLocalNetworks (that uses getifaddrs()) gets such addresses, that should not impact the callers because callers only work on addresses (i.e. struct in6_addr). Maybe, it would make sense to zero out this one in case there is some use case in future wherein sockaddresses containing link-local addresses are utilised on different layers where hash checks might end up failing and thereby having adverse effects. Even though sin6_flowinfo will be always 0, it wouldn't hurt to have both initialised to 0 here for consistency and uniformity. How does this sound? Setting zero in the object rather than input address. +++ Sockaddr& Sockaddr::operator=(const struct sockaddr_in6& addr) { set_length(sizeof(addr)); memcpy(&storage_, &addr, len_); storage_.in6.sin6_flowinfo = 0; storage_.in6.sin6_scope_id = 0; DCHECK_EQ(family(), AF_INET6); return *this; } +++ http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/socket-test.cc@103 PS11, Line 103: // Make 's_un' to be logically the same object as 's_in6', but reusing the : // Sockaddr::storage_ field from its prior incarnation. : ASSERT_OK(s_un.ParseString(kIpV6Addr, kPort)); > The whole point of this test scenario is to verify that the 'len_' field is Yes, I have verified HashCode() and BytewiseCompare() and both them do catch those mismatch cases. Here are some of the test cases if that is what you are pointing to: { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_flowinfo(12345); s_un6_1 = s_in6.ipv6_addr(); s_un6_1.set_flowinfo(1234); ASSERT_NE(s_in6.HashCode(), s_un6_1.HashCode()); ASSERT_FALSE(s_in6 == s_un6_1); } { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_flowinfo('a'); s_un6_1 = s_in6.ipv6_addr(); s_un6_1.set_flowinfo('b'); ASSERT_NE(s_in6.HashCode(), s_un6_1.HashCode()); ASSERT_FALSE(s_in6 == s_un6_1); } { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_flowinfo('a'); s_in6.set_scope_id(1234); s_un6_1 = s_in6.ipv6_addr(); s_un6_1.set_flowinfo('a'); s_un6_1.set_scope_id(1234); ASSERT_EQ(s_in6.HashCode(), s_un6_1.HashCode()); ASSERT_TRUE(s_in6 == s_un6_1); } { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_scope_id(1234); s_in6.set_flowinfo('a'); s_un6_1 = s_in6.ipv6_addr(); s_un6_1.set_scope_id('a'); s_un6_1.set_flowinfo(1234); ASSERT_NE(s_in6.HashCode(), s_un6_1.HashCode()); ASSERT_FALSE(s_in6 == s_un6_1); } { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_flowinfo(12345); s_un6_1 = s_in6.ipv6_addr(); ASSERT_TRUE(s_in6 == s_un6_1); } { Sockaddr s_un6_1; ASSERT_OK(s_un6_1.ParseUnixDomainPath(kPath)); s_in6.set_flowinfo('a'); s_un6_1 = s_in6.ipv6_addr(); ASSERT_TRUE(s_in6 == s_un6_1); } Note: These tests make use of set_flowinfo() and set_scope_id() public methods that were explicitly added to test those scenarios. I am not keen on adding these unit tests to upstream as I don't see much value in having them. If you are interested in adding such tests, I have one question - would having set methods be ok to do it along with a comment that these methods are for test purpose only? -- To view, visit http://gerrit.cloudera.org:8080/22984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22c773ffb2ff44b9cd765b546d6724ec5543586f Gerrit-Change-Number: 22984 Gerrit-PatchSet: 11 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 26 Aug 2025 12:30:27 +0000 Gerrit-HasComments: Yes
