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 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@325 PS6, Line 325: bool assignIPToClient(bool external) { Should use full camel-case: AssignIPToClient. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337 PS6, Line 337: if (GetLocalNetworks(&local_networks).ok()) { In the context of a test, I think we should hard stop if we encounter an unexpected situation, such as GetLocalNetworks failing. To that end, could you modify this function to return a Status, wrap this call in RETURN_NOT_OK, and communicate the success or failure of assignment via a bool* OUT parameter? Alternatively, you could treat an OK return result as meaning that all calls succeeded and a local IP was assigned, and a NotFound return result as a failure in assignment. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@338 PS6, Line 338: for (vector<Network>::iterator it = local_networks.begin(); : it != local_networks.end(); ++it) { You should use a range-based for loop, new to C++11: for (const auto& network : local_networks) { https://en.cppreference.com/w/cpp/language/range-for has more info. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341 PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) { Likewise, this should be encapsulated in a new function in Network. Maybe something like "IsLocalhost"? http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342 PS6, Line 342: char s[INET_ADDRSTRLEN]; : inet_ntop(AF_INET, &addr, s, INET_ADDRSTRLEN); Could you encapsulate this into a new function inside Network? Seems like Network::ParseCIDRString is similar (though it does the opposite work). http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@463 PS6, Line 463: Nit: extra space here. -- 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: 6 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: Fri, 15 Feb 2019 00:22:43 +0000 Gerrit-HasComments: Yes
