Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330 PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(&local_networks), ERROR, Use RETURN_NOT_OK_PREPEND instead; that's what we typically do. Or just RETURN_NOT_OK; it's just a test so the extra debuggability of the message prefix isn't important. http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@334 PS10, Line 334: if (!network.IsLoopBack()) { Nit: indentation http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@95 PS10, Line 95: // Returns true if addr is within 127.0.0.0/8 range. : static bool IsLoopBack(const uint32_t addr); : : // Returns dotted-decimal ('1.2.3.4') representation of IP address in addr. : static std::string AddrToString(const uint32_t addr); Nit: not really seeing why these args should be const; they're passed by value anyway so there's no effect on the caller's copy. http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@141 PS10, Line 141: bool IsLoopBack() const; Nit: existing code (see socket.{cc,h}) refers to it as "Loopback", so let's retain that convention. Above too. http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304 PS10, Line 304: return true; Do we need to further condition this local.IsAnyLocalAddress()? I'm guessing the answer is 'no' because it is assumed to be the case, but wanted to double check as it's something Alexey mentioned in KUDU-1900 (though Dan did not). -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 12 Gerrit-Owner: Greg Solovyev <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 16 Feb 2019 02:03:47 +0000 Gerrit-HasComments: Yes
