Ashwani Raina 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: (12 comments) 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 rep Yes 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 ref Done 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. Done 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 Done 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 muc I don't remember why I added this. Maybe, the code was still under testing and to make sure I am not hitting any garbage value post population failure, I may have left it there. This is not required since Query is going to fill it up with values in all good cases. Removed. 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()? Done http://gerrit.cloudera.org:8080/#/c/23021/1/src/kudu/util/net/diagnostic_socket-test.cc@202 PS1, Line 202: ListeningSocketIpV6 > BTW, did you try to run this test on a machine where IPv6 support is disabl I didn't try it. I believe IPv6 tests should fail at the time of bind, in such scenario. Eventually, we can have such detect conditions. I am planning to add more tests in subsequent changes that are going to use IPv6 addresses. At that time, it would make sense to have some common approach (preferably common code as well) for all such cases. Different OSes will have different ways to determine whether IPv6 is disabled or not. All those need to be taken into consideration. Do you see any issue in leaving these IPv6 tests as is (along with a comment stating the behaviour in such scenario) and add detection logic later? 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()? Done 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 Done 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? Done 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 usin Done 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 Readability-wise, it would be better to have explicit mention of both to avoid any confusion and additional effort to figure out whether code block does handle both case or not. Additionally, there are multiple places in the code where "IP address" is mentioned in comments and only IPv4 is handled there. Eventually, having "IP address" everywhere is desirable. Till then, explicit mention of IPv6 will provide assertive information about handling. -- 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: Tue, 29 Jul 2025 14:01:49 +0000 Gerrit-HasComments: Yes
