Alexey Serbin 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 12: (7 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: [](const testing::TestParamInfo<enum RpcSocketMode>& info) { : string test; : switch (info.param) { : case TCP_IPv4_NOSSL: : test = Substitute("TCP_IPv4_NoSSL"); : break; : case TCP_IPv6_NOSSL: : test = Substitute("TCP_IPv6_NoSSL"); : break; : default: : test = Substitute("INVALID_MODE"); : break; : } : return test; : }); nit: is it possible to consolidate this and another functor that's used at line 199, creating a stand-alone utility function and using it in both places? http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util-test.cc@382 PS11, Line 382: > I don't see any value in adding such test scenario as there is no case base Alright, that makes sense to me, thanks. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util-test.cc@412 PS11, Line 412: ASSERT_OK(network.ParseCIDRString("fd00:1234:5678::1/128")); > I don't see why ParseString won't work for IPv4-mapped IPv6 addresses. The OK, if there is coverage for that provided by other tests, it should enough then. 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: t HashCode() cons > You mean snake style (i.e. set_host_and_port) not camel style? camel style Right -- that should have been snake_style, not CamelStyle. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.h@95 PS11, Line 95: ; > Passing string_view as value is preferable as it is light-weight. As of PS12, it's still passed by const reference, not by value. Am I missing something or you are going to post another PS soon? 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: set_length(sizeof(addr)); : memcpy(&storage_, &addr, len_); : DCHECK_EQ(family(), AF_INET6); : return *this; : } : > I don't see a possibility of two fields (sin6_flowinfo and sin6_scope_id) i The point here is not exactly about the presence of garbage/non-initialized data in the fields, but rather any non-zero bits in the sin6_flowinfo and/or sin6_scope_id fields when 128bits of the actual IPv6 address are all the same. If that happens, KUDU-3352 is back, but this time in the IPv6 address domain. Your code in Sockaddr::ParseFromNumericHostPort calls set_length(sizeof(struct sockaddr_in6)) and explicitly zeroes out the sin6_flowinfo and sin6_scope_id fields. Here, the 'addr' might come from system calls or other places where we don't control the contents of the sin6_flowinfo and sin6_scope_id fields in the incoming sockaddr_in6 structure. Do we have any guarantee to have these fields zeroed-out in the structure coming as the 'addr' parameter? If not, then Sockaddr::HashCode() and Sockaddr::BytewiseCompare() cannot work as expected, and that's a problem. One manifestation of such a problem is described in KUDU-3353. Does this makes sense to you? 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: ASSERT_OK(s_in6.ParseString(kIpV6Addr, kPort)); : ASSERT_OK(s_un.ParseUnixDomainPath(kPath)); : > It should not matter whether current length of kPath is big enough to stamp The whole point of this test scenario is to verify that the 'len_' field is set correctly and the logic in BytewiseCompare()/HashCode() doesn't take into account irrelevant fields. If you think it doesn't matter whether current length of kPath is big enough to stamp over the relevant IPv6 fields in the sockaddr_in6 structure (namely sin6_flowinfo and sin6_scope_id fields), why did you add this new code at all? Let me ask a question: have you verified that this new test code you added into the SockaddrHashTest.ZeroPadding scenario catches the issue with BytewiseCompare()/HashCode() for SockAddr instances that contain exactly the same IPv6 address, but differ in the contents of the sin6_flowinfo and/or sin6_scope_id fields? -- 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: 12 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: Fri, 22 Aug 2025 19:02:44 +0000 Gerrit-HasComments: Yes
