Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22984 )
Change subject: [net] KUDU-1457 [1/n] Add support for IPv6 ...................................................................... Patch Set 1: (26 comments) http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/rpc/rpc-test.cc@160 PS1, Line 160: // TODO(araina): AcceptorPool thread may use diagnostic socket queries : // during the course of these tests. Return false once diagnostic socket : // handling is added for IPv6. Instead of messing up v4/v6 due to diagnostic socket, would it be more straightforward to simply disable diagnostic socket for IPv6 in the acceptor pools and not call corresponding parts of the tests here? Otherwise, no real testing for IPv6 RPC is going to happen here, it seems. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/dns_resolver-test.cc File src/kudu/util/net/dns_resolver-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/dns_resolver-test.cc@63 PS1, Line 63: if (AF_INET == addr.family()) { : EXPECT_TRUE(HasPrefixString(addr.ToString(), "127.")); : } else if (AF_INET6 == addr.family()) { : EXPECT_TRUE(HasPrefixString(addr.ToString(), "[::1]")); : } else { : FAIL() << "unexpected family:" << addr.family(); : } Is it possible to use switch() here? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: PS1: Please add test scenarios to verify how updated ParseString handles invalid input. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util-test.cc@211 PS1, Line 211: if (AF_INET == addr.family()) { : EXPECT_TRUE(HasPrefixString(addr.ToString(), "127.")); : } else if (AF_INET6 == addr.family()) { : EXPECT_TRUE(HasPrefixString(addr.ToString(), "[::1]")); : } else { : ASSERT_TRUE(false) << "unexpected family:" << addr.family(); : } Is it possible to use switch() here? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util-test.cc@218 PS1, Line 218: EXPECT_TRUE(HasSuffixString(addr.ToString(), ":12345")); Instead of ASSERT_TRUE(false), consider using FAIL() http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h@60 PS1, Line 60: // - [xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx]:port Are we going to support IPv4-mapped IPv6 addresses in text representation? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h@113 PS1, Line 113: static bool IsLoopback(sa_family_t family, uint128_t addr); Here and below: what is the rationale to have sa_family as the first parameter instead of the last? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h@172 PS1, Line 172: sa_family_t family_; For better memory alignment and lesser memory usage, move this field in the very end of the field list? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@a261 PS1, Line 261: Why not to have this check as DCHECK(), but update it to accept AF_INET6 as well? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@180 PS1, Line 180: StripWhiteSpace(&addr); : if (addr.empty()) { : return Status::InvalidArgument(Substitute("invalid address $0", addr)); : } Why not to do so in the very beginning of this method? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@199 PS1, Line 199: vector<string> p = strings::Split(addr, ":"); Is it possible to do this using std::string_view instead of allocating and copying over actual std::string instances? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@225 PS1, Line 225: host_ = addr; : host_.erase(host_.begin()); : host_.pop_back(); This is very inefficient. Instead, is it possible just to copy over the proper part of 'addr' into host_? You might use std::string_view or even existing std::string methods for doing so. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@422 PS1, Line 422: errno Error messages should be concise -- it's easier to read and it's less space used for the string in the binary. Maybe, update this to: "failed converting IP address to text form: $0" Capture errno first before using it here: Substitute() might produce another errno in rare cases (e.g., out of memory errors). Also, use ErrnoToString() to convert errno into readable error message. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@445 PS1, Line 445: ( nit: remove the enclosing parentheses http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@449 PS1, Line 449: // TODO(araina): For IPv6 addresses, probably prefix length is required : // to determine whether addresses are from same network. Since this isn't yet implemented, consider adding WIP for the description of your patch if you are still working on this. Otherwise, add DCHECK(false) to crash right here? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@472 PS1, Line 472: Net mask Please revert this back: it's called 'netmask', not 'net mask'. It's a well established term used in networking, 'subnet mask' or 'netmask': https://en.wikipedia.org/wiki/Subnet http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.cc@587 PS1, Line 587: ( remove useless parentheses? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h@57 PS1, Line 57: bool ipv4 = true) To distinguish between IPv4 and IPv6 address families, consider either using AF_INET style parameter or introduce a enum with good expressive enumerators. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h@116 PS1, Line 116: int If changing parameter type for 'set_port()', why not changing return type for this 'port()' method? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h@163 PS1, Line 163: ( remove the enclosing parentheses? 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@121 PS1, Line 121: sa_family_t family = strcount(hp.host(), ':') ? AF_INET6 : AF_INET; Don't we do this determination only once in HostPort already? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc@125 PS1, Line 125: &(storage_.in.sin_addr) I don't think it's not a good idea to touch this object's fields before successful return from inet_pton() since it's beneficial to rather have a transactional behavior here. In other words, if ParseFromNumericHostPort() succeeds, we get a valid Sockaddr object with all the fields filled as necessary, and if it fails, the original Sockaddr object isn't touched. Since there isn't a contract on whether inet_pton() is not touching its last argument if returning non-OK status, it's better to capture the address in a temporary variable first. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc@141 PS1, Line 141: } else if (AF_INET6 == family) { nit: add 'return Status::OK();' and remove 'else if'? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc@225 PS1, Line 225: set_length(sizeof(addr)); : memcpy(&storage_, &addr, len_); I'd expect to see the same dance as in ParseFromNumericHostPort()? For reasoning, take a look at the comment in Sockaddr::operator=(const struct sockaddr_in &) above. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/socket-test.cc@203 PS1, Line 203: 0.0.0.0:0 Consider introducing a constant for this wildcard address. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/socket-test.cc@206 PS1, Line 206: [::]:0 ditto -- 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: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 05 Jun 2025 18:48:28 +0000 Gerrit-HasComments: Yes
