Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23021 )
Change subject: [net] KUDU-1457 [3/n] Add support for IPv6 ...................................................................... Patch Set 1: Code-Review+1 (11 comments) Overall looks good, just a few nits. http://gerrit.cloudera.org:8080/#/c/23021/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23021/1//COMMIT_MSG@18 PS1, Line 18: Note: This patch has a dependency on following unmerged patch [1] that : is currently open for review. : : [1] https://gerrit.cloudera.org/c/22984/ This piece will be removed when submitting/committing this patch to the repo, right? http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc File src/kudu/util/net/diagnostic_socket-test.cc: http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@60 PS1, Line 60: ListeningSocket style nit: in this case, the name of the method should include verbs to reflect its main purpose/action, e.g. GetListeningSocketInfo() http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@82 PS1, Line 82: SimplePattern Ditto for the function/method name. http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@136 PS1, Line 136: if (src_addr.ipv4_addr().sin_addr.s_addr != entry.src_addr[0] || : INADDR_ANY != entry.dst_addr[0]) { : return false; : } nit for here and below: remove the 'if (!condition) {}' clause and replace with 'return condition;' http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@189 PS1, Line 189: = {} Why is this now necessary? If it's about zeroing out some fields, it's much better doing so in the constructor instead of doing so every time at call sites. http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@191 PS1, Line 191: ListeningSocket("127.254.254.254", &info); Wrap into NO_FATALS()? http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@205 PS1, Line 205: ListeningSocket("::1", &info) Wrap into NO_FATALS()? http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@208 PS1, Line 208: ASSERT_EQ nit for here and elsewhere: in EXPECT_EQ/ASSERT_EQ the expected value comes -- that way it's easier to comprehend the error message if this assertion ever triggers http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket.h File src/kudu/util/net/diagnostic_socket.h: http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket.h@59 PS1, Line 59: IPv4 Update this as well? http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket.cc File src/kudu/util/net/diagnostic_socket.cc: http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket.cc@185 PS1, Line 185: if (socket_src_addr.family() == AF_INET && socket_dst_addr.family() == AF_INET) { : const in_addr& src_ipv4 = socket_src_addr.ipv4_addr().sin_addr; : const auto src_port = socket_src_addr.port(); : const in_addr& dst_ipv4 = socket_dst_addr.ipv4_addr().sin_addr; : const auto dst_port = socket_dst_addr.port(); : : // All values in inet_diag_sockid and in_addr are in network byte order. : sock_id = (struct inet_diag_sockid) { : .idiag_sport = htons(src_port), : .idiag_dport = htons(dst_port), : .idiag_src = { src_ipv4.s_addr, 0, 0, 0, }, : .idiag_dst = { dst_ipv4.s_addr, 0, 0, 0, }, : .idiag_if = kWildcard, : .idiag_cookie = { kWildcard, kWildcard }, : }; : } else if (socket_src_addr.family() == AF_INET6 && socket_dst_addr.family() == AF_INET6) { : const in6_addr& src_ipv6 = socket_src_addr.ipv6_addr().sin6_addr; : const auto src_port = socket_src_addr.port(); : const in6_addr& dst_ipv6 = socket_dst_addr.ipv6_addr().sin6_addr; : const auto dst_port = socket_dst_addr.port(); : : // All values in inet_diag_sockid and in6_addr are in network byte order. : sock_id = (struct inet_diag_sockid) { : .idiag_sport = htons(src_port), : .idiag_dport = htons(dst_port), : .idiag_src = { src_ipv6.s6_addr32[0], : src_ipv6.s6_addr32[1], : src_ipv6.s6_addr32[2], : src_ipv6.s6_addr32[3], }, : .idiag_dst = { dst_ipv6.s6_addr32[0], : dst_ipv6.s6_addr32[1], : dst_ipv6.s6_addr32[2], : dst_ipv6.s6_addr32[3], }, : .idiag_if = kWildcard, : .idiag_cookie = { kWildcard, kWildcard }, : }; Instead of this boilerplate with duplicated code, could you rewrite it using ternary operator to assign the fields of the inet_diag_sockid structure (maybe, introducing temporary variables of type 'auto' if you need them)? http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket.cc@337 PS1, Line 337: IPv4 or IPv6 addresses nit for here and elsewhere: does it make sense to replace 'IPv4 and/or IPv6 address' with 'IP address' in the comments throughout the code? -- To view, visit http://gerrit.cloudera.org:8080/23021 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d78e241a8bb794465a613e7c0a11eea8f628849 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 1 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-Comment-Date: Fri, 18 Jul 2025 01:48:37 +0000 Gerrit-HasComments: Yes
