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: (2 comments) 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 : } Regardless where the source Sockaddr instance was created, to create an instance of a class, one of its constructors should have been called, unless some arbitrary memory is being cast via 'reinterpret_cast' to 'Sockaddr*'. The latter approach isn't safe without additional sanity checks and is not either needed nor used in the Kudu codebase at least for the Sockaddr class, AFAIK. The input parameter of this assignment operator is an instance of Sockaddr class. If the field zeroing for IPv6 address is done in all the constructors that create Sockaddr out of anything except of another Sockaddr instance, this code isn't needed. > No other code is going to zero out those fields. So, memcpy would just copy > those fields as is. In Socket::GetSocketAddress(), Socket::GetPeerAddress(), and Socket::Accept(), the field zeroing is performed by the code in the Sockaddr(const struct sockaddr&, socklen_t) constructor, and then for Sockaddr instances created out of IPv6 addresses memcpy() copies over the fields after the sanitization has been done already. Did you find an evidence that the required fields aren't zeroed out when this code is removed? http://gerrit.cloudera.org:8080/#/c/22984/14/src/kudu/util/net/sockaddr.cc@100 PS14, Line 100: is_initialized() && > On whether calling this method with 'len == 0' (which is similar to calling > above variant with 'other' including its 'len_' initialised to 0) is a > programming mistake or not question, probably you would be in a better > position to answer that as the test case was added as part of this commit: In the TimeIntervalsTest.SingleResponse test scenario this Sockaddr(const struct sockaddr&, socklen_t) contructor isn't invoked. AFAIK, only the default or copy constructors of Sockaddr are invoked for all the places involving RecordedResponse. For the default constructor, setting the 'len_' field to 0 is totally legal, of course. Meanwhile, the copy constructor invokes the assignment operator in its implementation, copying the 'len_' and the 'storage_' fields up to the 'len_' bytes. There, one might hit the DCHECK() in the Sockaddr::family(), but that code of the field zeroing isn't needed there, and I pointed to that fact at https://gerrit.cloudera.org/#/c/22984/14..14/src/kudu/util/net/sockaddr.cc@90 > So to ensure ntp-test don't fail due to DCHECK() failing because of zero > length in Sockaddr::family(), while initialising recorded responses from ntp > server. Although, ntp-test failed only for object assignment (i.e. lineno 90 > above) I added the check here as well to cover all the bases, just in case > there arises a need to use this variant also). What scenarios did you find failing in ntp-test because of this? I removed the 'is_initialized()' condition here and the code that zeroes out the fields in the assignment operator, and all the tests scenarios of ntp-test have passed in a DEBUG build as of PS15. That's exactly what I expected since there is 'set_lenght()' at line 98, and I don't think Sockaddr::Sockaddr(const struct Sockaddr&, socklen_t) is called anywhere with socklen_t value of 0 (and I think it shouldn't ever be called with len == 0). If there are any doubts, you could add DCHECK_NE(0, len) into Sockaddr::Sockaddr(const struct sockaddr&, socklen_t) implementation. > I would prefer adding a comment instead (added in PS15). Per my experience, bringing in and keeping not quite relevant and/or dead code adds extra costs. It's also confusing to people who are just trying to read and comprehend the code. Please consider removing this unless you have real, non-hypothetical reasons to keep it. -- 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: Mon, 01 Sep 2025 21:52:19 +0000 Gerrit-HasComments: Yes
