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 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@18 PS2, Line 18: #include <arpa/inet.h> : #include <ifaddrs.h> Is this approach portable to macOS? http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@331 PS2, Line 331: INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest, ::testing::ValuesIn( Would it be possible to use ::testing::Combine() to generate this more clearly? Could we structure the generators such that we get the right combinations (i.e. nearly the Cartesian product)? http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@376 PS2, Line 376: // an extnernal IP, so that the connection is not considered to be local external http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@378 PS2, Line 378: struct ifaddrs *ifap = nullptr; The initialization isn't necessary; getifaddrs() is going to overwrite 'ifap'. http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@380 PS2, Line 380: if (getifaddrs(&ifap) > -1) { Would be cleaner if this could early-out rather than introduce a new nested scope. Maybe decompose all of this into a helper method? http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@382 PS2, Line 382: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr || ifa->ifa_addr->sa_family != AF_INET) continue; Wrap, too long. http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@383 PS2, Line 383: struct sockaddr_in *addr_in = (struct sockaddr_in *)ifa->ifa_addr; Use C++-style casts here (static_cast or reinterpret_cast). http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@388 PS2, Line 388: FLAGS_local_ip_for_outbound_sockets.assign(s, INET_ADDRSTRLEN); Probably clearer as: FLAGS_foo = string(s, arraysize(s)); http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@392 PS2, Line 392: freeifaddrs(ifap); Use RAII principles: set up a SCOPED_CLEANUP that calls freeifaddrs() when it goes out of scope, declared just after getifaddrs() is known to have succeeded. http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc@302 PS2, Line 302: // Check if remote address is in 127.0.0.0/8 subnet Nit: terminate with a period. Applies to the new comments in the test too. http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc@303 PS2, Line 303: if( Nit: if (remote -- 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: 2 Gerrit-Owner: Greg Solovyev <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 13 Feb 2019 22:53:15 +0000 Gerrit-HasComments: Yes
