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 14: Code-Review+1 (6 comments) Almost there. PS14 looks good overall to me; thank you for addressing my concerns! My major concern in the recent patchesets for this changelist has been addressed (the potential issue that could trigger KUDU-3352), AFAIK. As of PS14, there are a few nits to address and I think this changelist should be good to go. Thanks a lot for revving this! 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@225 PS1, Line 225: return ""; : } > I have added explicit zeroing out of the two fields. Thank you! http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/sockaddr.cc@109 PS13, Line 109: // and properly initialized sockaddr_in's fields to BytewiseCompare() and > Not required here, instead doing it in these callees would make sense: Agreed -- minimizing the calls sites for 'field 'sanitization' makes sense; the end goal is to make sure the approach to fields sanitization is consistent for every code path. http://gerrit.cloudera.org:8080/#/c/22984/14/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/14/src/kudu/util/net/sockaddr.cc@90 PS14, Line 90: if (is_initialized() && family() == AF_INET6) { : storage_.in6.sin6_flowinfo = 0; // Flow-label & traffic class : storage_.in6.sin6_scope_id = 0; // Scope identifier : } Do we need this block here? I'd think that these two fields were guaranteed zeros in any source instance of Sockaddr that had sockaddr_in6 underneath due to the rest of the code in PS14. So, we can rely on memcpy() doing its job at line 89, no? http://gerrit.cloudera.org:8080/#/c/22984/14/src/kudu/util/net/sockaddr.cc@100 PS14, Line 100: is_initialized() && What's the idea behind adding this extra condition? This looks a bit strange to me because 'is_initialized()' simply checks 'len_' against 0 while there is 'set_length()' just above. So, is_initialized() here cannot evaluate to 'false' unless 'len == 0'. From the other side, calling this method with 'len == 0' could be a programming mistake on its own, and the existing DCHECK() in Sockaddr::family() would catch it in DEBUG builds if not this extra condition. Am I missing something? If there is some non-trival complications that require adding this extra condition, please add a comment to explain, otherwise remove it? 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@103 PS11, Line 103: ASSERT_OK(s_in6.ParseString(kIpV6Addr, kPort)); : ASSERT_OK(s_un.ParseUnixDomainPath(kPath)); : > Length of kPath is 34 bytes which is enough to overwrite the fields of in6 OK, then we have good coverage with the newly added code in this test scenario. If there is a doubt in proper sanitization of the data in sockaddr_in6 structures stored in Sockaddr for a particular call sequence, we can always add more sub-scenarios here, I guess. Thank you for addressing my concern! http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/socket-test.cc@288 PS13, Line 288: // IPv6. : DoTestServerConnect("::1", AF_INET6); : // IPv4-mapped IPv6 address. : DoTestServerConnect("::ffff:127.0.0.1", AF_INET6); > I have responded to similar comment here: Sure -- we can have it as-is for now, and add the IPv6 detection logic later in a separate patch, if ever needed. As an alternative, we might consider start requiring IPv6 support for Apache Kudu development activities. I'm fine with either of these approaches; PS14 looks good to me in this regard. -- 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: 14 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: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Fri, 29 Aug 2025 02:19:44 +0000 Gerrit-HasComments: Yes
