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 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 that provides the description of the testing mode. It's much easier to read and interpret the results of the tests with human-readable names for the tests. 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 instantiations -- it has to be updated. 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 addition to running on plain ones? If so, please update the description above, and also add a functor that provides human-readable description for each of the test modes. 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 argument at each of the call sites? 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-case hex digits are parsed as expected, and the result IPv6 addresses are the same? 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 representation of IPv4-mapped IPv6 addresses isn't supported by ParseString, at least to be aware of what Status code it would return if fed with such a string? 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. 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 getters and setters; this should rather be something like SetHostAndPort() or similar. 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 reference here? 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 http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@362 PS11, Line 362: nit: fix the indent? 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 after it's clear all the rest of the fields are valid? Essentially, the idea is to either set all the fields if the CIDR string was successfully parsed, or doesn't touch any of the fields if there was an error. 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 passing the arguments and constructing an object in-place http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/net_util.cc@629 PS11, Line 629: Network nit: 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'? 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 could see that format is supported in the ParseString implementation, but I might misunderstood the code. 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 instead of counting all? 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' field? If so, shouldn't we protect against that to avoid having too much fun later on with BytewiseCompare() and HashCode()? 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 case of IPv6-releated structures 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 the related fields of IPv6-related structures stored under the Sockaddr::storage_ umbrella? If it makes sense, consider adding corresponding static asserts (or just ASSERTs) for to check for that. -- 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: Fri, 08 Aug 2025 18:36:05 +0000 Gerrit-HasComments: Yes
