Ashwani Raina 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 15: (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: // For some unit tests (e.g. ntp-test), zeroed out Sockaddr objects : // are used that can land here during the object assignment. So, check : // the family of only the properly initialised objects. : i > Do we need this block here? I'd think that these two fields were guarantee What if source instance was populated from "outside" calls like Socket::GetSocketAddress(), Socket::GetPeerAddress(), and Socket::Accept() (as you suggested here: https://gerrit.cloudera.org/#/c/22984/1/src/kudu/util/net/sockaddr.cc@226)? No other code is going to zero out those fields. So, memcpy would just copy those fields as is. http://gerrit.cloudera.org:8080/#/c/22984/14/src/kudu/util/net/sockaddr.cc@100 PS14, Line 100: dr::Sockaddr(const > What's the idea behind adding this extra condition? Yes I agree with you on that - it does not make sense. But, I had to put these checks because at some places in ntp-test, zeroed-out Sockaddr objects are being used. 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). 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: +++ commit 12922d8d89416f362c55945d8e50797776731ee2 Author: Alexey Serbin <[email protected]> Date: Sun Sep 29 17:42:43 2019 -0700 [clock] add unit tests for FindIntersection() function: --- TEST(TimeIntervalsTest, SingleResponse) { // Zero clock skew: this is something theoretical, but anyways. // In case of zero clock skew, regardless of the difference between the // capture time and current time, the result error is not widening, // and the result interval is drifting along with current time. { // Zero error. { const vector<RecordedResponse> responses = { { 0, 1, 0, }, >>>>> }; +++ To make it a little more readable, I guess we can have two variants of Sockaddr::family() with a new one that can be unsafe access. I would prefer adding a comment instead (added in PS15). -- 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: 15 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 13:19:29 +0000 Gerrit-HasComments: Yes
