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: (20 comments) http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/rpc/rpc-test.cc@200 PS11, Line 200: testing::Values(TCP_IPv4_SSL, TCP_IPv4_NOSSL, : TCP_IPv6_SSL, TCP_IPv6_NOSSL, : UNIX_SSL, UNIX_NOSSL)); > Please add back (i.e. implement it for the RpcSocketMode enum) the function Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/rpc/rpc-test.cc@2049 PS11, Line 2049: // All the tests run without SSL on TCP sockets: TestRpcSocketTxRxQueue inherits : // from TestRpc, and the latter is parameterized. Running with SSL doesn't make : // much sense since it's the same in this context: all the action happens : // at the TCP level, and RPC connection negotiation doesn't happen. > This comment is now outdated after changing the set of parameters for test Not applicable now, as the original coverage remains i.e. running without SSL on socket. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/rpc/rpc-test.cc@2054 PS11, Line 2054: TCP_IPv4_SSL, TCP_IPv4_NOSSL, : TCP_IPv6_SSL, TCP_IPv6_NOSSL, : UNIX_SSL, UNIX_NOSSL > Did you find that running this for SSL-enabled sockets has any value in add Come to think of it, there is little to no significant value addition in running these tests on SSL-enabled sockets because SSL operates on different layer and it is agnostic to IP protocol. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/dns_resolver-test.cc File src/kudu/util/net/dns_resolver-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/dns_resolver-test.cc@53 PS11, Line 53: Sockaddr addr > Why not to pass this as const reference? Is it a requirement to copy the a Done 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: 2405:201:d01e:e861:4234:4a42:40e7:8e40 > Any chance to have a test sub-scenario that verifies both upper- and lower- I don't see any value in adding such test scenario as there is no case based logic/modifications done to the input string. Refer this: https://gerrit.cloudera.org/#/c/22984/6/src/kudu/util/net/net_util-test.cc@326 In the interest of time, I have added a couple of tests for upper- and lower-cases, anyway. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util-test.cc@412 PS11, Line 412: > Does it makes sense to add a test scenario to explicitly show that string r I don't see why ParseString won't work for IPv4-mapped IPv6 addresses. The parsing logic does necessary string operations to get host and port (if specified) out of the input string. Given a standard representation of IPv4-mapped IPv6 address, ParseString should be able to represent the same in 128-bit format. I have added a couple more tests that bind a socket to IPv4-mapped IPv6 address and a client connects to this server. The test is TestServerConnect inside socket-test.cc file. 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@52 PS11, Line 52: // Split a string_view into vector of string_view objects (pointing : // to original memory) separated by a given delimiter. : static std::vector<std::string_view> SplitStringView(std::string_view s, : char delimiter); > What is this for? I couldn't find its definition and usage as of PS11. Removed. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.h@95 PS11, Line 95: const std::string_view& host > If passing string_view by value in other places, why to pass if by referenc Passing string_view as value is preferable as it is light-weight. Changed to value. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.h@95 PS11, Line 95: set_host_and_port > style nit: low-case camel-style member function names are for single field You mean snake style (i.e. set_host_and_port) not camel style? camel style is mixed case with first letter of each word in capitals. Refer this: https://google.github.io/styleguide/cppguide.html#Naming I was not aware that convention is different for single field getters and setters. Anyway, I have changed this to camel case. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@239 PS11, Line 239: either > nit: it's either Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@362 PS11, Line 362: > nit: fix the indent? Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@505 PS11, Line 505: family_ = sockaddr.family(); > nit: to make this more atomic, move the assignment of address family field Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@623 PS11, Line 623: Network > nit: could remove Network -- the semantics of various emplace_xxx is about Good point! Done http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@629 PS11, Line 629: Network > nit: ditto ditto http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.h@53 PS11, Line 53: Common construct > nit: what is 'common construct'? This variant is actually constructing from a generic socket address. Changed the comment. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.h@88 PS11, Line 88: any other acceptable IPv6 representation > So, we now support IPv4-mapped IPv6 addresses as of PS11? I'm not sure I c ParseString doesn't distinguish between a normal IPv6 address and an IPv4-mapped IPv6 address. Given an IPv4-mapped IPv6, ParseString is expected to fill host and port appropriately as it would do for a normal IPv6 address. I have added couple of tests in socket-test.cc for IPv4-mapped IPv6 address as well. 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@124 PS11, Line 124: strcount(hp.host(), ':') > nit: use hp.host().find(':') to find just first instance of a column instea Done 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; : } > Wouldn't it copy some garbage over from 'addr' into zeroed 'sin6_flowinfo' I don't see a possibility of two fields (sin6_flowinfo and sin6_scope_id) in 'addr' containing garbage values if there is a valid length representing the address bytes. The 'len_' (representing valid bytes in 'storage_') field should either be 0 (if object was initialised with default ctor) or some valid value (if initialised with wildcard or ParseFromNumericHostPort). Both BytewiseCompare() and HashCode() work on length of valid bytes (i.e. len_). If it is 0, default hash prime would be applicable for both inputs and for BytewiseCompare() as well, minimum length is taken into consideration. We are safe for both cases. 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@54 PS11, Line 54: the implementation of : // the Sockaddr class should zero out the zero padding field : // sockaddr_in::sin_zero > nit: please update this comment to mention the details of similar dances in Done 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)); > Just to double check: is current length of kPath enough here to stamp over It should not matter whether current length of kPath is big enough to stamp over IPv6 fields. len_ decides validity of bytes inside storage_ structure. For subsequent HashCode call as well, it relies on len_ member to decide number of bytes to be taken into consideration when finding the hash value. -- 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: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 18 Aug 2025 13:47:35 +0000 Gerrit-HasComments: Yes
