From a8a1005addbd6dd1432272497e9e05f337310c1b Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Mon, 14 Nov 2016 16:18:32 -0500 Subject: [PATCH 1/6] All c-adapter changes --- CMakeLists.txt | 22 +++++++ README.md | 61 +++++++++++++++--- src/README.md | 17 +++++ src/main.cc | 85 ++++++++++++++++++++++++ src/tcpinfo_c_adapter.c | 139 ++++++++++++++++++++++++++++++++++++++++ src/tcpinfo_c_adapter.h | 47 ++++++++++++++ src/tcpinfo_lib.cc | 19 ++++++ src/tcpinfo_lib.h | 3 + src/tcpinfo_lib_test.cc | 137 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 522 insertions(+), 8 deletions(-) create mode 100644 src/README.md create mode 100644 src/main.cc create mode 100644 src/tcpinfo_c_adapter.c create mode 100644 src/tcpinfo_c_adapter.h create mode 100644 src/tcpinfo_lib_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 5fe80df..fc6b610 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,6 +66,7 @@ PROTOBUF_GENERATE_CPP(PROTO_SRCS PROTO_HDRS ${SRC_DIR}/tcpinfo.proto) add_library(tcpinfo_lib ${SRC_DIR}/tcpinfo_lib.cc ${SRC_DIR}/tcpinfo_lib.h + ${SRC_DIR}/tcpinfo_c_adapter.h ${SRC_DIR}/tcpinfo_c_adapter.c ${PROTO_HDRS} ${PROTO_SRCS}) add_dependencies(tcpinfo_lib iproute2) @@ -89,6 +90,17 @@ target_link_libraries(tcpinfo_proto_test add_test(tcpinfo_proto tcpinfo_proto_test) +add_executable(tcpinfo_lib_test + ${SRC_DIR}/tcpinfo_lib_test.cc) +add_dependencies(tcpinfo_lib_test googletest) + +target_link_libraries(tcpinfo_lib_test + ${GTEST_LIBS_DIR}/libgtest.a + ${GTEST_LIBS_DIR}/libgtest_main.a + tcpinfo_lib) + +add_test(tcpinfo_lib tcpinfo_lib_test) + ################### connection_cache library ######################## add_library(connection_cache_lib ${SRC_DIR}/connection_cache.cc ${SRC_DIR}/connection_cache.h) @@ -107,3 +119,13 @@ target_link_libraries(connection_cache_test add_test(connection_cache connection_cache_test) +################### tcpinfo polling proto ######################## +add_executable(poll + ${SRC_DIR}/main.cc + ${SRC_DIR}/tcpinfo_c_adapter.c) +add_dependencies(poll iproute2 tcpinfo_lib) +target_link_libraries(poll + ${IPROUTE2_LIBS_DIR}/libnetlink.a + ${IPROUTE2_LIBS_DIR}/libutil.a + connection_cache_lib + tcpinfo_lib) diff --git a/README.md b/README.md index 11c636b..2a8ab22 100644 --- a/README.md +++ b/README.md @@ -19,16 +19,55 @@ mkdir build cd build cmake .. && cmake --build . && ctest -V ``` +When make finishes, there should be a directory tree under .../build that +contains all config, code, object files, libraries, and binaries. -# Dependencies -There are several dependencies that are handled through add_directory(...). The -CMake files for these are under ext/, and they are invoked as needed. +# Travis-CI integration +.travis.yml provides config for building within travis-ci. It includes a gcc +config that also produces coverage data for coveralls, and a clang config that +builds optimized code. -The install_protobuf.sh script is used for building and installing google protobuf as needed. +# Prerequisites +## General +General requirements for building this repository +``` +sudo apt-get install autoconf automake libtool curl make g++ unzip +``` +## clang +The build is configured to use clang. If you don't have it installed, you +can try commenting out the two lines near the top of the CMakeLists.txt file. + +## protobuf compiler +CMake apparently uses pkg-config to discover the protobuf compiler. But it +appears that when installed with apt-get install protobuf-compiler, (at least +on gobuntu) the pkg-config .pc file is not installed. +So, to make everything work smoothly, run from the main directory: +``` +install-protobuf.sh +``` +This requires sudo privileges, and you may be prompted for your password for +the install and ldconfig steps. + +FYI, if you already have a protobuf compiler visible to the pkg-config tools, +this script will do nothing. + +If you later want to revert your system, you will need to cd to the protobuf +directory, and: +``` +sudo make uninstall +``` +You will like then also need to: +``` +sudo apt-get uninstall protobuf-compiler +sudo apt-get install protobuf-compiler +``` + +# Dependency on iproute2 and gtest +The ext/iproute2 and ext/gtest directories provide rules for downloading +and building the dependencies. They are incorporated with add_subdirectory +and should be downloaded and built when needed. They should show up under +build/ext/... -# Travis-CI integration -.travis.yml provides config for building within travis-ci. Currently only gcc -config is working, but will hopefully add clang config later. # Source tree ``` @@ -38,16 +77,22 @@ config is working, but will hopefully add clang config later. │   ├── gtest │  │ └── CMakeLists.txt │   └── iproute2 -│  └── CMakeLists.txt +│   └── CMakeLists.txt ├── install-protobuf.sh ├── LICENSE +├── pre-commit ├── README.md └── src ├── connection_cache.cc ├── connection_cache.h ├── connection_cache_test.cc + ├── main.cc + ├── README.md + ├── tcpinfo_c_adapter.c + ├── tcpinfo_c_adapter.h ├── tcpinfo_lib.cc ├── tcpinfo_lib.h + ├── tcpinfo_lib_test.cc ├── tcpinfo.proto └── tcpinfo_proto_test.cc ``` diff --git a/src/README.md b/src/README.md new file mode 100644 index 0000000..34f8ed7 --- /dev/null +++ b/src/README.md @@ -0,0 +1,17 @@ +# Components +## tcpinfo.proto +... contains the data structures for all tcp_info and netlink related code. + +## tcpinfo_lib +... contains the primary interface for interacting with the + +## connection_cache +... keeps a cache of data for each connection seen so far. + +## tcpinfo_c_adapter +... contains the low level code that makes calls to the netlink library and +iproute2 code. + +## main.cc +... contains a simple demo polling program. + diff --git a/src/main.cc b/src/main.cc new file mode 100644 index 0000000..211ddb5 --- /dev/null +++ b/src/main.cc @@ -0,0 +1,85 @@ +/********************************************************************** + * This captures 10 minutes of connection data. + * Compressed with gzip, takes about 23 bytes / record. + * Compressed with bzip2, takes about 17 bytes / record. (15.5:1) + **********************************************************************/ + +#include +#include +#include + +#include "tcpinfo_lib.h" // Poller + +namespace { +void output(const std::string& nlmsg, int protocol, std::string tag) { + std::ofstream out; + out.open("nldata", std::ofstream::app | std::ofstream::binary); + int len = nlmsg.size(); + for (int i = 0; i < len; ++i) { + out.put(nlmsg.at(i)); + } + out.close(); + + // TODO(gfr) This should be optional. + auto* hdr = (const struct nlmsghdr*)nlmsg.c_str(); + mlab::netlink::TCPInfoParser parser; + auto proto = parser.ParseNLMsg(hdr, mlab::netlink::Protocol(protocol)); + printf("%s\n", proto.ShortDebugString().c_str()); + const google::protobuf::EnumDescriptor* enum_desc = + mlab::netlink::TCPState_descriptor(); + + fprintf(stderr, "%5d %s %s\n", + proto.inet_diag_msg().sock_id().source().port(), + tag.c_str(), + enum_desc->value( + proto.inet_diag_msg().state()) + ->options().GetExtension(mlab::netlink::name).c_str()); +} + +void output_hash(int protocol, const std::string& old_msg, const std::string& new_msg) { + output(old_msg, protocol, "#"); +} + +void on_new_state(int protocol, const std::string& old_msg, + const std::string& new_msg) { + // Always output old data when we see new state! + if (!old_msg.empty()) { + auto old_state = mlab::netlink::GetStateFromStr(old_msg); + + // Output old state if it is ESTABLISHED. + if (old_state == mlab::netlink::TCPState::ESTABLISHED) { + output(old_msg, protocol, "*"); + } + } + + // For all states EXCEPT ESTABLISHED, output the state immediately. + if (mlab::netlink::GetStateFromStr(new_msg) + != mlab::netlink::TCPState::ESTABLISHED) { + output(new_msg, protocol, " "); + } +} +} // anonymous namespace + +extern mlab::netlink::TCPInfoPoller g_poller_; + +int main(int argc, char* argv[]) { + using std::chrono::steady_clock; + using std::chrono::duration; + + auto start = steady_clock::now(); + auto one_minute = duration(60); + + // NOTE: This is rarely firing, because we almost always see some other + // state as the connection is closing. + g_poller_.OnClose(output_hash, {mlab::netlink::TCPState::ESTABLISHED}); + // This is handling all the output. + g_poller_.OnNewState(on_new_state); + + int count = 0; + while (steady_clock::now() < start + 10*one_minute) { + g_poller_.PollOnce(); + count++; + } + fprintf(stderr, "Rate was %d polls/sec\n", count / 600); + return 0; +} diff --git a/src/tcpinfo_c_adapter.c b/src/tcpinfo_c_adapter.c new file mode 100644 index 0000000..354e06b --- /dev/null +++ b/src/tcpinfo_c_adapter.c @@ -0,0 +1,139 @@ +/* + * This file contains code that calls C functions defined in the netlink + * and related libraries, and iproute2 code. It is challenging to + * implement in C++ because of linkage issues. + * + * Derived from iproute2 ss.c. Forked from net-next in Sept 2016. + * Much of the content has been left close in form to the original + * to make it clearer what the relationships are with the ss.c code. + * Most of the ss.c code is not needed, and therefore stripped out. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + * + * Original ss.c Author: Alexey Kuznetsov, + */ + +#include "tcpinfo_c_adapter.h" + +#include +#include +#include + +#include "libnetlink.h" + +// Would rather use tcp_states.h, which is what idiag_states needs, but that +// seems to be only in the kernel headers. linux/tcp.h in gobuntu doesn't +// have these either. 8-( +enum { + SS_UNKNOWN, + SS_ESTABLISHED, + SS_SYN_SENT, + SS_SYN_RECV, // Excluded from SS_CONN + SS_FIN_WAIT1, + SS_FIN_WAIT2, + SS_TIME_WAIT, // Excluded from SS_CONN + SS_CLOSE, // Excluded from SS_CONN + SS_CLOSE_WAIT, + SS_LAST_ACK, + SS_LISTEN, // Excluded from SS_CONN + SS_CLOSING, + SS_MAX +}; + +#define MAGIC_SEQ 123456 + +// family is expected to be PF_INET or PF_INET6 +// protocol expected to be IPPROTO_TCP +static int sockdiag_send(int family, int fd, int protocol) { + struct sockaddr_nl nladdr = {.nl_family = AF_NETLINK}; + struct { + struct nlmsghdr nlh; + struct inet_diag_req_v2 r; + } req = { + .nlh = + { + .nlmsg_type = SOCK_DIAG_BY_FAMILY, + .nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST, + .nlmsg_seq = MAGIC_SEQ, + .nlmsg_len = sizeof(req), + }, + }; + struct msghdr msg; + struct iovec iov[3]; // TODO: Why 3 ?? + int iovlen = 1; + + memset(&req.r, 0, sizeof(req.r)); + req.r.sdiag_family = family; + req.r.sdiag_protocol = protocol; + req.r.idiag_states = 1 << SS_ESTABLISHED; + // DO NOT SUBMIT +// req.r.idiag_states = 0; + /**************************************************************** + * With these additional flags, we occasionally last state we see + * is usually LAST_ACK or TIME_WAIT, and occasionally we see one of + * the FIN_WAIT states. The LAST_ACK and FIN_WAIT1 states have full + * information, but FIN_WAIT2 and TIME_WAIT have only the inet_diag_msg. + * + * Incidentally, dropping ESTABLISHED only raises the polling rate + * from about 1000/sec to about 1600/sec. + *****************************************************************/ + req.r.idiag_states |= 1 << SS_SYN_SENT; + req.r.idiag_states |= 1 << SS_SYN_RECV; + req.r.idiag_states |= 1 << SS_CLOSE_WAIT; + req.r.idiag_states |= 1 << SS_CLOSING; + req.r.idiag_states |= 1 << SS_CLOSE; + req.r.idiag_states |= 1 << SS_LAST_ACK; + req.r.idiag_states |= 1 << SS_FIN_WAIT1; + req.r.idiag_states |= 1 << SS_FIN_WAIT2; +// req.r.idiag_states |= 1 << SS_TIME_WAIT; + + // show_mem + req.r.idiag_ext |= (1 << (INET_DIAG_MEMINFO - 1)); + req.r.idiag_ext |= (1 << (INET_DIAG_SKMEMINFO - 1)); + + // show_tcpinfo + req.r.idiag_ext |= (1 << (INET_DIAG_INFO - 1)); + req.r.idiag_ext |= (1 << (INET_DIAG_VEGASINFO - 1)); + req.r.idiag_ext |= (1 << (INET_DIAG_CONG - 1)); + + iov[0] = (struct iovec){.iov_base = &req, .iov_len = sizeof(req)}; + + msg = (struct msghdr){ + .msg_name = (void *)&nladdr, + .msg_namelen = sizeof(nladdr), + .msg_iov = iov, + .msg_iovlen = iovlen, + }; + + if (sendmsg(fd, &msg, 0) < 0) { + close(fd); + return -1; + } + + return 0; +} + +// Returns error code from sockdiag_send or rtnl_dump_filter. +// 0: successful +// < 0: error +int fetch_tcpinfo(rtnl_filter_t callback) { + int err = 0; + struct rtnl_handle rth; + struct inet_diag_arg arg = {.protocol = IPPROTO_TCP}; + if (rtnl_open_byproto(&rth, 0, NETLINK_SOCK_DIAG)) return -1; + + rth.dump = MAGIC_SEQ; + rth.dump_fp = NULL; // TODO(gfr) Do we want to support dump_fp? + + if ((err = sockdiag_send(PF_INET, rth.fd, IPPROTO_TCP))) goto Exit; + if ((err = rtnl_dump_filter(&rth, callback, &arg))) goto Exit; + if ((err = sockdiag_send(PF_INET6, rth.fd, IPPROTO_TCP))) goto Exit; + if ((err = rtnl_dump_filter(&rth, callback, &arg))) goto Exit; + +Exit: + rtnl_close(&rth); + return err; +} diff --git a/src/tcpinfo_c_adapter.h b/src/tcpinfo_c_adapter.h new file mode 100644 index 0000000..94ddce2 --- /dev/null +++ b/src/tcpinfo_c_adapter.h @@ -0,0 +1,47 @@ +// Copyright 2016 measurement-lab +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// This file contains declarations that are used by both .c files and .cc +// files. When included in .cc files, it must be enclosed in an extern "C" +// block. +// +// fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy +// access to all the C libraries. +// update_record(...) is implemented in tcpinfo_lib.cc, because it requires +// access to C++ functions. + +#ifndef TCPINFO_C_ADAPTER_H_ +#define TCPINFO_C_ADAPTER_H_ + +#include +#include +#include + +#include "libnetlink.h" + +struct inet_diag_arg { + int protocol; +}; + +// Fetch all tcp socket connections in established state. Implemention in +// tcpinfo_c_adapter.c +// has rtnl_filter_t signature, and is passed to rtnl_dump_filter. +int fetch_tcpinfo(rtnl_filter_t callback); + +// rtnl_filter_t function to handle each result, passed into fetch_tcpinfo() +// function. Implementation in tcpinfo_lib.cc +int update_record(const struct sockaddr_nl *addr, + struct nlmsghdr *nlh, void *arg); + +#endif // TCPINFO_C_ADAPTER_H_ diff --git a/src/tcpinfo_lib.cc b/src/tcpinfo_lib.cc index 9ccb222..cdfaa03 100644 --- a/src/tcpinfo_lib.cc +++ b/src/tcpinfo_lib.cc @@ -25,6 +25,7 @@ extern "C" { #include #include "libnetlink.h" +#include "tcpinfo_c_adapter.h" // declaration of update_record. } namespace mlab { @@ -343,6 +344,11 @@ void TCPInfoPoller::on_close_wrapper(int protocol, void TCPInfoPoller::PollOnce() { using namespace std::placeholders; + // TODO(gfr) - Pass in update_record bound to `this`? + if (fetch_tcpinfo(update_record)) { + // TODO - handle errors. Not at all clear what to do. Probably + // just LOG(FATAL). + } tracker_.VisitMissingRecords(std::bind(&TCPInfoPoller::on_close_wrapper, this, _1, _2, _3)); tracker_.increment_round(); @@ -428,3 +434,16 @@ TCPState GetStateFromStr(const std::string& nlmsg) { mlab::netlink::TCPInfoPoller g_poller_; +extern "C" { +// Signature must match rtnl_filter_t. +// All args must be non-null. +int update_record(const struct sockaddr_nl *addr, struct nlmsghdr *nlh, + void *arg) { + auto *diag_arg = (struct inet_diag_arg *)arg; + auto *msg = (struct inet_diag_msg *)NLMSG_DATA(nlh); + // This has to be tied to a specific instance of the TCPInfoPoller. 8-( + g_poller_.Stash( + msg->idiag_family, diag_arg->protocol, msg->id, nlh); + return 0; +} +} // extern "C" diff --git a/src/tcpinfo_lib.h b/src/tcpinfo_lib.h index 0cab863..96b3453 100644 --- a/src/tcpinfo_lib.h +++ b/src/tcpinfo_lib.h @@ -166,6 +166,9 @@ class TCPInfoPoller { const struct nlmsghdr* nlh); private: + FRIEND_TEST(TCPInfoLib, DISABLED_Basic); + FRIEND_TEST(TCPInfoLib, CAdapters); + FRIEND_TEST(TCPInfoLib, OnClose); FRIEND_TEST(Poller, StashAndOnClose); // For testing purposes. diff --git a/src/tcpinfo_lib_test.cc b/src/tcpinfo_lib_test.cc new file mode 100644 index 0000000..cd26209 --- /dev/null +++ b/src/tcpinfo_lib_test.cc @@ -0,0 +1,137 @@ +// Copyright 2016 measurement-lab +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// TODO(gfr) The include order matters. If tcpinfo_lib.h is included later, +// we get obscure compiler errors processing tcpinfo.pb.h. +#include "tcpinfo_lib.h" + +#include + +#include "gtest/gtest.h" +#include "connection_cache.h" + +extern "C" { +#include // IPPROTO_TCP +#include // AF_INET* + +#include "tcpinfo_c_adapter.h" +} + +extern mlab::netlink::TCPInfoPoller g_poller_; + +// 0 0 2620:0:1003:413:ac8f:7971:3973:b48e:38625 +// 2607:f8b0:4006:80d::200e:https timer:(keepalive 31sec 0), uid:148024, +// ino:89694138, sk:0, <->, skmem:(r0,rb369280,t0,tb87552,f0,w0,o0,bl0) ts, +// sack, cubic, wscale:7/7, rto:208, rtt:8.5/11, ato:40, mss:1398, cwnd:19, +// ssthresh:18, send 25.0M,bps, lastsnd:239268, lastrcv:239268, lastack:13972, +// rcv_rtt:36, rcv_space:28800 +std::string raw1( + "\x10\x01\x00\x00\x14\x00\x02\x00\x40\xE2\x01\x00\xA8\x4B\x00\x00\x0A\x01" + "\x02\x00\x96\xE1\x01\xBB\x26\x20\x00\x00\x10\x03\x04\x13\xAC\x8F\x79\x71" + "\x39\x73\xB4\x8E\x26\x07\xF8\xB0\x40\x06\x08\x0D\x00\x00\x00\x00\x00\x00" + "\x20\x0E\x00\x00\x00\x00\x00\x1F\xF8\x35\x00\x88\xFF\xFF\x6C\x79\x00\x00" + "\x00\x00\x00\x00\x00\x00\x00\x00\x38\x42\x02\x00\xBA\x9F\x58\x05\x05\x00" + "\x08\x00\x00\x00\x00\x00\x14\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00" + "\x00\x00\x00\x00\x00\x00\x00\x00\x24\x00\x07\x00\x00\x00\x00\x00\x80\xA2" + "\x05\x00\x00\x00\x00\x00\x00\x56\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00" + "\x00\x00\x00\x00\x00\x00\x00\x00\x6C\x00\x02\x00\x01\x00\x00\x00\x00\x07" + "\x77\x00\x80\x2C\x03\x00\x40\x9C\x00\x00\x76\x05\x00\x00\x4A\x02\x00\x00" + "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" + "\x00\x00\xA4\xA6\x03\x00\x00\x00\x00\x00\xA4\xA6\x03\x00\x94\x36\x00\x00" + "\xDC\x05\x00\x00\xD8\x7D\x00\x00\x34\x21\x00\x00\xF8\x2A\x00\x00\x12\x00" + "\x00\x00\x13\x00\x00\x00\x94\x05\x00\x00\x03\x00\x00\x00\xA0\x8C\x00\x00" + "\x80\x70\x00\x00\x00\x00\x00\x00\x0A\x00\x04\x00\x63\x75\x62\x69\x63\x00" + "\x00\x00", + 0x110); + +namespace mlab { +// Simple visitor that just counts the visited records. +int visit_count = 0; +bool saw_msg1 = false; + +void visitor(int protocol, const std::string& old_msg, + const std::string& new_msg) { + saw_msg1 |= (old_msg == raw1); + visit_count++; +} + +namespace netlink { + +// When running without ESTABLISHED state, this generally produces +// no results, so it is useless. +TEST(TCPInfoLib, DISABLED_Basic) { + visit_count = 0; + TCPInfoPoller poller; + poller.PollOnce(); // This should populate the poller's tracker object. + + // Not quite intended use, but useful testing hack. + g_poller_.GetTracker()->VisitMissingRecords(visitor); + g_poller_.GetTracker()->increment_round(); + + // CAUTION: This may be flaky since it depends on number of + // TCP connections on the machine. + EXPECT_GT(visit_count, 2); +} + +TEST(TCPInfoLib, CAdapters) { + visit_count = 0; + auto* hdr = (struct nlmsghdr*)raw1.c_str(); + struct inet_diag_arg diag_arg = { + .protocol = IPPROTO_TCP, + }; + + update_record(nullptr, hdr, (void *)&diag_arg); + g_poller_.GetTracker()->increment_round(); + // Not quite intended use, but useful testing hack. + g_poller_.GetTracker()->VisitMissingRecords(visitor); + EXPECT_EQ(visit_count, 1); +} + +TEST(TCPInfoLib, ToString) { + EndPoint ep4; + ep4.set_ip("\x01\x02\x03\x04"); + ep4.set_port(1234); + EXPECT_EQ(ToString(ep4), "1.2.3.4:1234"); + + EndPoint ep6; + ep6.set_ip("\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F\x10"); + ep6.set_port(4321); + EXPECT_EQ(ToString(ep6), "[102:304:506:708:90a:b0c:d0e:f10]:4321"); +} + +TEST(TCPInfoLib, OnClose) { + visit_count = 0; + saw_msg1 = false; + + // Insert some artificial items into the cache. + auto* hdr = (struct nlmsghdr*)raw1.c_str(); + struct inet_diag_arg diag_arg = { + .protocol = IPPROTO_TCP, + }; + update_record(nullptr, hdr, (void *)&diag_arg); + g_poller_.GetTracker()->increment_round(); + + // Now set up the poller, and configure to run visitor on close. + g_poller_.OnClose(visitor); + + // When we run PollOnce, it should iterate over cache entries, and call + // visitor for each old item that isn't updated. The visitor should see + // raw, so saw_msg1 should be true. + g_poller_.PollOnce(); + EXPECT_EQ(visit_count, 1); + EXPECT_TRUE(saw_msg1); +} + +} // namespace netlink +} // namespace mlab From 72e307bbd6be042d66cb6de2beb65aa523107f07 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Tue, 15 Nov 2016 20:02:35 -0500 Subject: [PATCH 2/6] small changes to fix unit tests --- src/main.cc | 51 +++++++++++++++++++++++++++++---------- src/tcpinfo_lib.cc | 4 +-- src/tcpinfo_lib.h | 2 +- src/tcpinfo_lib_test.cc | 42 +++++++++++++++++--------------- src/tcpinfo_proto_test.cc | 14 ++++++++--- 5 files changed, 74 insertions(+), 39 deletions(-) diff --git a/src/main.cc b/src/main.cc index 211ddb5..8d80c5a 100644 --- a/src/main.cc +++ b/src/main.cc @@ -1,7 +1,10 @@ /********************************************************************** + * Demo polling implementation. + * * This captures 10 minutes of connection data. - * Compressed with gzip, takes about 23 bytes / record. - * Compressed with bzip2, takes about 17 bytes / record. (15.5:1) + * Raw nlmsg data compressed with bzip2 takes about 15 bytes/record. + * Wire format protos compressed with bzip2 also takes about 15 bytes/record. + * bzip2 compressed proto.ShortDebugString takes about 16 bytes/record. **********************************************************************/ #include @@ -11,7 +14,8 @@ #include "tcpinfo_lib.h" // Poller namespace { -void output(const std::string& nlmsg, int protocol, std::string tag) { +// Dump just raw nlmsg, without protocol info. +void DumpNlMsg(const std::string& nlmsg) { std::ofstream out; out.open("nldata", std::ofstream::app | std::ofstream::binary); int len = nlmsg.size(); @@ -19,15 +23,21 @@ void output(const std::string& nlmsg, int protocol, std::string tag) { out.put(nlmsg.at(i)); } out.close(); +} + +void DumpProto(const mlab::netlink::TCPDiagnosticsProto& proto) { + std::ofstream out; + out.open("protodata", std::ofstream::app | std::ofstream::binary); + proto.SerializeToOstream(&out); + out.close(); // TODO(gfr) This should be optional. - auto* hdr = (const struct nlmsghdr*)nlmsg.c_str(); - mlab::netlink::TCPInfoParser parser; - auto proto = parser.ParseNLMsg(hdr, mlab::netlink::Protocol(protocol)); printf("%s\n", proto.ShortDebugString().c_str()); +} + +void DumpSummary(const mlab::netlink::TCPDiagnosticsProto& proto, std::string tag) { const google::protobuf::EnumDescriptor* enum_desc = mlab::netlink::TCPState_descriptor(); - fprintf(stderr, "%5d %s %s\n", proto.inet_diag_msg().sock_id().source().port(), tag.c_str(), @@ -36,17 +46,33 @@ void output(const std::string& nlmsg, int protocol, std::string tag) { ->options().GetExtension(mlab::netlink::name).c_str()); } -void output_hash(int protocol, const std::string& old_msg, const std::string& new_msg) { +void output(const std::string& nlmsg, int protocol, std::string tag) { + DumpNlMsg(nlmsg); + + auto proto = mlab::netlink::TCPInfoParser().ParseNLMsg( + nlmsg, mlab::netlink::Protocol(protocol)); + + DumpProto(proto); + DumpSummary(proto, tag); +} + +// Output, using '#' as tag for summary. +void on_close(int protocol, const std::string& old_msg, + const std::string& new_msg) { output(old_msg, protocol, "#"); } +// Output each new state we see, except for ESTABLISHED, which should only +// be output when it is the old state. +// PREREQ: old_msg and new_msg should have different TCPState values. void on_new_state(int protocol, const std::string& old_msg, const std::string& new_msg) { // Always output old data when we see new state! if (!old_msg.empty()) { auto old_state = mlab::netlink::GetStateFromStr(old_msg); - // Output old state if it is ESTABLISHED. + // Output old state if it is ESTABLISHED, since it is NOT output as new + // state. if (old_state == mlab::netlink::TCPState::ESTABLISHED) { output(old_msg, protocol, "*"); } @@ -69,10 +95,9 @@ int main(int argc, char* argv[]) { auto start = steady_clock::now(); auto one_minute = duration(60); - // NOTE: This is rarely firing, because we almost always see some other - // state as the connection is closing. - g_poller_.OnClose(output_hash, {mlab::netlink::TCPState::ESTABLISHED}); - // This is handling all the output. + // NOTE: This fires only occasionally, on very short lived connections. + // Usually, we see some other OnNewState before the connection closes. + g_poller_.OnClose(on_close, {mlab::netlink::TCPState::ESTABLISHED}); g_poller_.OnNewState(on_new_state); int count = 0; diff --git a/src/tcpinfo_lib.cc b/src/tcpinfo_lib.cc index cdfaa03..4307af8 100644 --- a/src/tcpinfo_lib.cc +++ b/src/tcpinfo_lib.cc @@ -111,6 +111,7 @@ void ParseInetDiagMsg(struct inet_diag_msg* r, InetDiagMsgProto* proto) { void ParseBBRInfo(const struct rtattr* rta, BBRInfoProto* proto) { const auto* bbr = (const struct tcp_bbr_info*)RTA_DATA(rta); auto bw = (((unsigned long long)bbr->bbr_bw_hi) << 32) + bbr->bbr_bw_lo; + fprintf(stderr, "HI: %x\n", bbr->bbr_bw_hi); if (bw > 0) proto->set_bw(bw); if (bbr->bbr_min_rtt > 0) proto->set_min_rtt(bbr->bbr_min_rtt); if (bbr->bbr_pacing_gain > 0) proto->set_pacing_gain(bbr->bbr_pacing_gain); @@ -346,8 +347,7 @@ void TCPInfoPoller::PollOnce() { using namespace std::placeholders; // TODO(gfr) - Pass in update_record bound to `this`? if (fetch_tcpinfo(update_record)) { - // TODO - handle errors. Not at all clear what to do. Probably - // just LOG(FATAL). + // TODO(gfr) LOG(FATAL) ?? } tracker_.VisitMissingRecords(std::bind(&TCPInfoPoller::on_close_wrapper, this, _1, _2, _3)); diff --git a/src/tcpinfo_lib.h b/src/tcpinfo_lib.h index 96b3453..f0fe03c 100644 --- a/src/tcpinfo_lib.h +++ b/src/tcpinfo_lib.h @@ -166,7 +166,7 @@ class TCPInfoPoller { const struct nlmsghdr* nlh); private: - FRIEND_TEST(TCPInfoLib, DISABLED_Basic); + FRIEND_TEST(TCPInfoLib, Basic); FRIEND_TEST(TCPInfoLib, CAdapters); FRIEND_TEST(TCPInfoLib, OnClose); FRIEND_TEST(Poller, StashAndOnClose); diff --git a/src/tcpinfo_lib_test.cc b/src/tcpinfo_lib_test.cc index cd26209..2c530ff 100644 --- a/src/tcpinfo_lib_test.cc +++ b/src/tcpinfo_lib_test.cc @@ -36,7 +36,7 @@ extern mlab::netlink::TCPInfoPoller g_poller_; // sack, cubic, wscale:7/7, rto:208, rtt:8.5/11, ato:40, mss:1398, cwnd:19, // ssthresh:18, send 25.0M,bps, lastsnd:239268, lastrcv:239268, lastack:13972, // rcv_rtt:36, rcv_space:28800 -std::string raw1( +std::string msg1( "\x10\x01\x00\x00\x14\x00\x02\x00\x40\xE2\x01\x00\xA8\x4B\x00\x00\x0A\x01" "\x02\x00\x96\xE1\x01\xBB\x26\x20\x00\x00\x10\x03\x04\x13\xAC\x8F\x79\x71" "\x39\x73\xB4\x8E\x26\x07\xF8\xB0\x40\x06\x08\x0D\x00\x00\x00\x00\x00\x00" @@ -62,38 +62,36 @@ bool saw_msg1 = false; void visitor(int protocol, const std::string& old_msg, const std::string& new_msg) { - saw_msg1 |= (old_msg == raw1); + saw_msg1 |= (old_msg == msg1); visit_count++; } namespace netlink { -// When running without ESTABLISHED state, this generally produces -// no results, so it is useless. -TEST(TCPInfoLib, DISABLED_Basic) { - visit_count = 0; - TCPInfoPoller poller; - poller.PollOnce(); // This should populate the poller's tracker object. - - // Not quite intended use, but useful testing hack. - g_poller_.GetTracker()->VisitMissingRecords(visitor); - g_poller_.GetTracker()->increment_round(); - - // CAUTION: This may be flaky since it depends on number of - // TCP connections on the machine. - EXPECT_GT(visit_count, 2); +TEST(TCPInfoLib, Basic) { + g_poller_.ClearCache(); + g_poller_.PollOnce(); // This should populate the poller's tracker object. + // Most machines will have at least a few non-local connections. + EXPECT_GT(g_poller_.ClearCache(), 2); } TEST(TCPInfoLib, CAdapters) { + g_poller_.ClearCache(); visit_count = 0; - auto* hdr = (struct nlmsghdr*)raw1.c_str(); + auto* hdr = (struct nlmsghdr*)msg1.data(); struct inet_diag_arg diag_arg = { .protocol = IPPROTO_TCP, }; + g_poller_.GetTracker()->VisitMissingRecords(visitor); + ASSERT_EQ(visit_count, 0); + update_record(nullptr, hdr, (void *)&diag_arg); - g_poller_.GetTracker()->increment_round(); + // Not quite intended use, but useful testing hack. + // Increment the round, so all records are out of date. + // Then visit missing (all) records. + g_poller_.GetTracker()->increment_round(); g_poller_.GetTracker()->VisitMissingRecords(visitor); EXPECT_EQ(visit_count, 1); } @@ -111,23 +109,27 @@ TEST(TCPInfoLib, ToString) { } TEST(TCPInfoLib, OnClose) { + g_poller_.ClearCache(); visit_count = 0; saw_msg1 = false; // Insert some artificial items into the cache. - auto* hdr = (struct nlmsghdr*)raw1.c_str(); + auto* hdr = (struct nlmsghdr*)msg1.data(); struct inet_diag_arg diag_arg = { .protocol = IPPROTO_TCP, }; update_record(nullptr, hdr, (void *)&diag_arg); - g_poller_.GetTracker()->increment_round(); // Now set up the poller, and configure to run visitor on close. g_poller_.OnClose(visitor); + g_poller_.GetTracker()->increment_round(); // When we run PollOnce, it should iterate over cache entries, and call // visitor for each old item that isn't updated. The visitor should see // raw, so saw_msg1 should be true. + // UNFORTUNATELY, it will also poll the tcpinfo from the network stack, so + // IF there is an open connection with the same key, it could mask our + // artificial connection. g_poller_.PollOnce(); EXPECT_EQ(visit_count, 1); EXPECT_TRUE(saw_msg1); diff --git a/src/tcpinfo_proto_test.cc b/src/tcpinfo_proto_test.cc index 7368a99..d9ecdf9 100644 --- a/src/tcpinfo_proto_test.cc +++ b/src/tcpinfo_proto_test.cc @@ -529,6 +529,13 @@ void Print(const struct nlmsghdr* nlh) { } } // anonymous namespace +using namespace std::placeholders; + +#define POLL_WITHOUT_FETCH(poller) \ + poller.GetTracker()->VisitMissingRecords( \ + std::bind(&TCPInfoPoller::on_close_wrapper, &poller, _1, _2, _3)); \ + poller.GetTracker()->increment_round(); \ + TEST(Poller, StashAndOnClose) { // Stash and onclose in another file. @@ -557,8 +564,8 @@ TEST(Poller, StashAndOnClose) { // All three of these are new states. EXPECT_EQ(on_new_state_count, 3); - // For now this just visits missing rounds, and increments round(); - p.PollOnce(); + // Just visits missing rounds, and increment round; + POLL_WITHOUT_FETCH(p); EXPECT_EQ(on_close_count, 0); // Try another round. Same message should NOT trigger on_new_state. @@ -570,8 +577,9 @@ TEST(Poller, StashAndOnClose) { } EXPECT_EQ(on_new_state_count, 3); + // Just visits missing rounds, and increment round; // This one should cause the on_close_ to be invoked for two messages. - p.PollOnce(); + POLL_WITHOUT_FETCH(p); // We put in one ESTABLISHED, and one OTHER. We should only see ESTABLISHED. EXPECT_EQ(on_close_count, 1); EXPECT_FALSE(new_msg_not_empty); // OnClose should always have empty messages. From a447e1c604896b7794abc638658b94228121b229 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Tue, 15 Nov 2016 21:08:34 -0500 Subject: [PATCH 3/6] tweak comments in tcpinfo_c_adapter.h --- src/tcpinfo_c_adapter.h | 23 ++++++++++++----------- src/tcpinfo_lib.cc | 1 - src/tcpinfo_lib_test.cc | 7 +++---- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/tcpinfo_c_adapter.h b/src/tcpinfo_c_adapter.h index 94ddce2..9ed2fc5 100644 --- a/src/tcpinfo_c_adapter.h +++ b/src/tcpinfo_c_adapter.h @@ -12,14 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -// This file contains declarations that are used by both .c files and .cc -// files. When included in .cc files, it must be enclosed in an extern "C" -// block. -// -// fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy -// access to all the C libraries. -// update_record(...) is implemented in tcpinfo_lib.cc, because it requires -// access to C++ functions. +/**************************************************************************** +* This file contains declarations that are used by both .c files and .cc +* files. When included in .cc files, it must be enclosed in an extern "C" +* block. +* +* fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy +* access to all the C libraries. +* update_record(...) is implemented in tcpinfo_lib.cc, because it requires +* access to C++ functions. +****************************************************************************/ #ifndef TCPINFO_C_ADAPTER_H_ #define TCPINFO_C_ADAPTER_H_ @@ -34,9 +36,8 @@ struct inet_diag_arg { int protocol; }; -// Fetch all tcp socket connections in established state. Implemention in -// tcpinfo_c_adapter.c -// has rtnl_filter_t signature, and is passed to rtnl_dump_filter. +// Fetch all tcp socket connections. Implemention in tcpinfo_c_adapter.c +// `callback` has rtnl_filter_t signature, and is passed to rtnl_dump_filter. int fetch_tcpinfo(rtnl_filter_t callback); // rtnl_filter_t function to handle each result, passed into fetch_tcpinfo() diff --git a/src/tcpinfo_lib.cc b/src/tcpinfo_lib.cc index 4307af8..41ab714 100644 --- a/src/tcpinfo_lib.cc +++ b/src/tcpinfo_lib.cc @@ -111,7 +111,6 @@ void ParseInetDiagMsg(struct inet_diag_msg* r, InetDiagMsgProto* proto) { void ParseBBRInfo(const struct rtattr* rta, BBRInfoProto* proto) { const auto* bbr = (const struct tcp_bbr_info*)RTA_DATA(rta); auto bw = (((unsigned long long)bbr->bbr_bw_hi) << 32) + bbr->bbr_bw_lo; - fprintf(stderr, "HI: %x\n", bbr->bbr_bw_hi); if (bw > 0) proto->set_bw(bw); if (bbr->bbr_min_rtt > 0) proto->set_min_rtt(bbr->bbr_min_rtt); if (bbr->bbr_pacing_gain > 0) proto->set_pacing_gain(bbr->bbr_pacing_gain); diff --git a/src/tcpinfo_lib_test.cc b/src/tcpinfo_lib_test.cc index 2c530ff..99fc558 100644 --- a/src/tcpinfo_lib_test.cc +++ b/src/tcpinfo_lib_test.cc @@ -12,14 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -// TODO(gfr) The include order matters. If tcpinfo_lib.h is included later, -// we get obscure compiler errors processing tcpinfo.pb.h. -#include "tcpinfo_lib.h" - #include #include "gtest/gtest.h" #include "connection_cache.h" +// TODO(gfr) The include order matters. If tcpinfo_lib.h is included later, +// we get obscure compiler errors processing tcpinfo.pb.h. +#include "tcpinfo_lib.h" extern "C" { #include // IPPROTO_TCP From 3fae7b98be119e03a3a027e79fc1f7835c59ee0d Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 17 Nov 2016 11:16:53 -0500 Subject: [PATCH 4/6] more comment and doc cleanup. --- README.md | 30 +++++++------------- src/tcpinfo_c_adapter.c | 61 +++++++++++++++++++++-------------------- src/tcpinfo_c_adapter.h | 14 +++++----- 3 files changed, 48 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 2a8ab22..aa77373 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,13 @@ sudo apt-get install build-essential autoconf automake make cmake sudo apt-get install clang g++ libtool curl unzip bison flex ``` +## clang +The build is configured to use clang. If you want to use a different compiler, +export CXX_COMPILER and C_COMPILER. + # Building ``` -git clone git@github.com:gfr10598/tcpinfo_lib +git clone git@github.com:mlab/tcpinfo_lib cd tcpinfo_lib mkdir build cd build @@ -27,36 +31,23 @@ contains all config, code, object files, libraries, and binaries. config that also produces coverage data for coveralls, and a clang config that builds optimized code. -# Prerequisites -## General -General requirements for building this repository -``` -sudo apt-get install autoconf automake libtool curl make g++ unzip -``` -## clang -The build is configured to use clang. If you don't have it installed, you -can try commenting out the two lines near the top of the CMakeLists.txt file. - ## protobuf compiler CMake apparently uses pkg-config to discover the protobuf compiler. But it appears that when installed with apt-get install protobuf-compiler, (at least on gobuntu) the pkg-config .pc file is not installed. -So, to make everything work smoothly, run from the main directory: -``` -install-protobuf.sh -``` +So, to make everything work smoothly, cmake .. will run install-protobuf.sh, +which will install it from github if pkg-config doesn't have a suitable config. + This requires sudo privileges, and you may be prompted for your password for the install and ldconfig steps. -FYI, if you already have a protobuf compiler visible to the pkg-config tools, -this script will do nothing. - If you later want to revert your system, you will need to cd to the protobuf directory, and: ``` sudo make uninstall ``` -You will like then also need to: +To restore the original protobuf-compiler (if you had one), you may also need +to: ``` sudo apt-get uninstall protobuf-compiler sudo apt-get install protobuf-compiler @@ -68,7 +59,6 @@ and building the dependencies. They are incorporated with add_subdirectory and should be downloaded and built when needed. They should show up under build/ext/... - # Source tree ``` ├── .travis.yml diff --git a/src/tcpinfo_c_adapter.c b/src/tcpinfo_c_adapter.c index 354e06b..78234b0 100644 --- a/src/tcpinfo_c_adapter.c +++ b/src/tcpinfo_c_adapter.c @@ -1,21 +1,20 @@ -/* - * This file contains code that calls C functions defined in the netlink - * and related libraries, and iproute2 code. It is challenging to - * implement in C++ because of linkage issues. - * - * Derived from iproute2 ss.c. Forked from net-next in Sept 2016. - * Much of the content has been left close in form to the original - * to make it clearer what the relationships are with the ss.c code. - * Most of the ss.c code is not needed, and therefore stripped out. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version - * 2 of the License, or (at your option) any later version. - * - * Original ss.c Author: Alexey Kuznetsov, - */ - +// Copyright 2016 measurement-lab +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// This code is loosely modeled on misc/ss.c code from the iproute2 +// library by Alexey Kuznetsov, circa September 2016. +// #include "tcpinfo_c_adapter.h" #include @@ -68,18 +67,16 @@ static int sockdiag_send(int family, int fd, int protocol) { memset(&req.r, 0, sizeof(req.r)); req.r.sdiag_family = family; req.r.sdiag_protocol = protocol; + + // With these additional flags, the last state we see is usually LAST_ACK, + // TIME_WAIT, or one of the FIN_WAIT states. The LAST_ACK and FIN_WAIT1 + // states have full information, but FIN_WAIT2 and TIME_WAIT have only the + // inet_diag_msg. + // Since we don't ALWAYS see a final state when a connection ends, we request + // and monitor ESTABLISHED records to avoid missing connections (though we may + // still miss some). Including ESTABLISHED drops the polling rate from about + // 1600/sec to about 1000/sec on a modern 3 GHz machine. req.r.idiag_states = 1 << SS_ESTABLISHED; - // DO NOT SUBMIT -// req.r.idiag_states = 0; - /**************************************************************** - * With these additional flags, we occasionally last state we see - * is usually LAST_ACK or TIME_WAIT, and occasionally we see one of - * the FIN_WAIT states. The LAST_ACK and FIN_WAIT1 states have full - * information, but FIN_WAIT2 and TIME_WAIT have only the inet_diag_msg. - * - * Incidentally, dropping ESTABLISHED only raises the polling rate - * from about 1000/sec to about 1600/sec. - *****************************************************************/ req.r.idiag_states |= 1 << SS_SYN_SENT; req.r.idiag_states |= 1 << SS_SYN_RECV; req.r.idiag_states |= 1 << SS_CLOSE_WAIT; @@ -88,7 +85,7 @@ static int sockdiag_send(int family, int fd, int protocol) { req.r.idiag_states |= 1 << SS_LAST_ACK; req.r.idiag_states |= 1 << SS_FIN_WAIT1; req.r.idiag_states |= 1 << SS_FIN_WAIT2; -// req.r.idiag_states |= 1 << SS_TIME_WAIT; + req.r.idiag_states |= 1 << SS_TIME_WAIT; // show_mem req.r.idiag_ext |= (1 << (INET_DIAG_MEMINFO - 1)); @@ -96,7 +93,11 @@ static int sockdiag_send(int family, int fd, int protocol) { // show_tcpinfo req.r.idiag_ext |= (1 << (INET_DIAG_INFO - 1)); + + // congestion info req.r.idiag_ext |= (1 << (INET_DIAG_VEGASINFO - 1)); + req.r.idiag_ext |= (1 << (INET_DIAG_DCTCPINFO - 1)); + req.r.idiag_ext |= (1 << (INET_DIAG_BBRINFO - 1)); req.r.idiag_ext |= (1 << (INET_DIAG_CONG - 1)); iov[0] = (struct iovec){.iov_base = &req, .iov_len = sizeof(req)}; diff --git a/src/tcpinfo_c_adapter.h b/src/tcpinfo_c_adapter.h index 9ed2fc5..d05214d 100644 --- a/src/tcpinfo_c_adapter.h +++ b/src/tcpinfo_c_adapter.h @@ -13,14 +13,14 @@ // limitations under the License. /**************************************************************************** -* This file contains declarations that are used by both .c files and .cc -* files. When included in .cc files, it must be enclosed in an extern "C" -* block. +* This file contains declarations that are used by both .c files and .cc +* files. When included in .cc files, it must be enclosed in an extern "C" +* block. * -* fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy -* access to all the C libraries. -* update_record(...) is implemented in tcpinfo_lib.cc, because it requires -* access to C++ functions. +* fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy +* access to C libraries. +* update_record(...) is implemented in tcpinfo_lib.cc, because it requires +* access to C++ functions. ****************************************************************************/ #ifndef TCPINFO_C_ADAPTER_H_ From 96459320d59e47ab3259f2e152e286f3a68cbf88 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 17 Nov 2016 14:03:06 -0500 Subject: [PATCH 5/6] various fixes for PR review comments --- src/README.md | 11 +++++------ src/main.cc | 12 +++++------- src/tcpinfo_c_adapter.c | 6 +++++- src/tcpinfo_lib.h | 8 ++++++++ src/tcpinfo_proto_test.cc | 22 +++++++++++----------- 5 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/README.md b/src/README.md index 34f8ed7..2ad8347 100644 --- a/src/README.md +++ b/src/README.md @@ -1,17 +1,16 @@ # Components ## tcpinfo.proto -... contains the data structures for all tcp_info and netlink related code. +Data structures for all tcp_info and netlink related code. ## tcpinfo_lib -... contains the primary interface for interacting with the +Utility library providing high level api to netlink services. ## connection_cache -... keeps a cache of data for each connection seen so far. +Component for caching data for each active (non-local) connection. ## tcpinfo_c_adapter -... contains the low level code that makes calls to the netlink library and -iproute2 code. +Low level code that makes calls to the netlink library and iproute2 code. ## main.cc -... contains a simple demo polling program. +Simple demo polling program illustrating use of the tcpinfo_lib library. diff --git a/src/main.cc b/src/main.cc index 8d80c5a..83fa026 100644 --- a/src/main.cc +++ b/src/main.cc @@ -46,7 +46,7 @@ void DumpSummary(const mlab::netlink::TCPDiagnosticsProto& proto, std::string ta ->options().GetExtension(mlab::netlink::name).c_str()); } -void output(const std::string& nlmsg, int protocol, std::string tag) { +void Output(const std::string& nlmsg, int protocol, std::string tag) { DumpNlMsg(nlmsg); auto proto = mlab::netlink::TCPInfoParser().ParseNLMsg( @@ -59,7 +59,7 @@ void output(const std::string& nlmsg, int protocol, std::string tag) { // Output, using '#' as tag for summary. void on_close(int protocol, const std::string& old_msg, const std::string& new_msg) { - output(old_msg, protocol, "#"); + Output(old_msg, protocol, "#"); } // Output each new state we see, except for ESTABLISHED, which should only @@ -67,21 +67,19 @@ void on_close(int protocol, const std::string& old_msg, // PREREQ: old_msg and new_msg should have different TCPState values. void on_new_state(int protocol, const std::string& old_msg, const std::string& new_msg) { - // Always output old data when we see new state! + // Output old data when it's state is ESTABLISHED. if (!old_msg.empty()) { auto old_state = mlab::netlink::GetStateFromStr(old_msg); - // Output old state if it is ESTABLISHED, since it is NOT output as new - // state. if (old_state == mlab::netlink::TCPState::ESTABLISHED) { - output(old_msg, protocol, "*"); + Output(old_msg, protocol, "*"); } } // For all states EXCEPT ESTABLISHED, output the state immediately. if (mlab::netlink::GetStateFromStr(new_msg) != mlab::netlink::TCPState::ESTABLISHED) { - output(new_msg, protocol, " "); + Output(new_msg, protocol, " "); } } } // anonymous namespace diff --git a/src/tcpinfo_c_adapter.c b/src/tcpinfo_c_adapter.c index 78234b0..3cd9d6e 100644 --- a/src/tcpinfo_c_adapter.c +++ b/src/tcpinfo_c_adapter.c @@ -42,7 +42,11 @@ enum { SS_MAX }; -#define MAGIC_SEQ 123456 +// Magic number used for nlmsg_seq. Doesn't really matter since we are +// running synchronously (???). +// TODO(gfr) For thoroughness, should this be a sequence instead of a constant? +// iproute2 ss.c just uses a constant. +#define MAGIC_SEQ 0x3A41B852 // family is expected to be PF_INET or PF_INET6 // protocol expected to be IPPROTO_TCP diff --git a/src/tcpinfo_lib.h b/src/tcpinfo_lib.h index f0fe03c..8933453 100644 --- a/src/tcpinfo_lib.h +++ b/src/tcpinfo_lib.h @@ -36,6 +36,12 @@ extern "C" { namespace mlab { namespace netlink { +class TCPInfoPoller; +namespace test { +// Test support function that required friend status. +void VisitAndIncrement(TCPInfoPoller* poller); +} // namespace test + // Key interface elements: // 1. Poll all tcp connections, and maintain cache of measurements for each // connection. @@ -166,6 +172,8 @@ class TCPInfoPoller { const struct nlmsghdr* nlh); private: + friend void test::VisitAndIncrement(TCPInfoPoller* poller); + FRIEND_TEST(TCPInfoLib, Basic); FRIEND_TEST(TCPInfoLib, CAdapters); FRIEND_TEST(TCPInfoLib, OnClose); diff --git a/src/tcpinfo_proto_test.cc b/src/tcpinfo_proto_test.cc index d9ecdf9..5c500dc 100644 --- a/src/tcpinfo_proto_test.cc +++ b/src/tcpinfo_proto_test.cc @@ -529,16 +529,16 @@ void Print(const struct nlmsghdr* nlh) { } } // anonymous namespace +namespace test { using namespace std::placeholders; - -#define POLL_WITHOUT_FETCH(poller) \ - poller.GetTracker()->VisitMissingRecords( \ - std::bind(&TCPInfoPoller::on_close_wrapper, &poller, _1, _2, _3)); \ - poller.GetTracker()->increment_round(); \ +void VisitAndIncrement(TCPInfoPoller* poller) { + poller->GetTracker()->VisitMissingRecords( + std::bind(&TCPInfoPoller::on_close_wrapper, poller, _1, _2, _3)); + poller->GetTracker()->increment_round(); +} +} // namespace test TEST(Poller, StashAndOnClose) { -// Stash and onclose in another file. - TCPInfoPoller p; p.OnClose(on_close, {mlab::netlink::TCPState::ESTABLISHED}); p.OnNewState(on_new_state); // always call for new states. @@ -564,8 +564,8 @@ TEST(Poller, StashAndOnClose) { // All three of these are new states. EXPECT_EQ(on_new_state_count, 3); - // Just visits missing rounds, and increment round; - POLL_WITHOUT_FETCH(p); + // Visit stale records, and increment round; + test::VisitAndIncrement(&p); EXPECT_EQ(on_close_count, 0); // Try another round. Same message should NOT trigger on_new_state. @@ -577,9 +577,9 @@ TEST(Poller, StashAndOnClose) { } EXPECT_EQ(on_new_state_count, 3); - // Just visits missing rounds, and increment round; + // Visit stale records, and increment round; // This one should cause the on_close_ to be invoked for two messages. - POLL_WITHOUT_FETCH(p); + test::VisitAndIncrement(&p); // We put in one ESTABLISHED, and one OTHER. We should only see ESTABLISHED. EXPECT_EQ(on_close_count, 1); EXPECT_FALSE(new_msg_not_empty); // OnClose should always have empty messages. From ae23c27bd7510272e72935d18208fd32d4523be1 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 17 Nov 2016 19:21:26 -0500 Subject: [PATCH 6/6] Fix messy extern "C" related issues. --- src/connection_cache.cc | 2 +- src/tcpinfo.proto | 8 +++++--- src/tcpinfo_c_adapter.c | 1 + src/tcpinfo_c_adapter.h | 33 ++++++++++++--------------------- src/tcpinfo_lib.cc | 10 +++++----- src/tcpinfo_lib.h | 6 ++++++ src/tcpinfo_lib_test.cc | 12 ++---------- src/tcpinfo_proto_test.cc | 4 ++-- 8 files changed, 34 insertions(+), 42 deletions(-) diff --git a/src/connection_cache.cc b/src/connection_cache.cc index bc495ea..b0c3a56 100644 --- a/src/connection_cache.cc +++ b/src/connection_cache.cc @@ -21,7 +21,7 @@ extern "C" { #include -#include +#include // For AF_INET } namespace mlab { diff --git a/src/tcpinfo.proto b/src/tcpinfo.proto index 0e64d7b..cfc98e5 100644 --- a/src/tcpinfo.proto +++ b/src/tcpinfo.proto @@ -82,10 +82,12 @@ enum TCPState { // from tcp_states.h message InetDiagMsgProto { enum AddressFamily { + // NOTE: these are equivalent to AF_... in socket.h, but cannot have the + // same names since those are macros and will cause collisions. // There are many other families, but for now we only care about these. - AF_UNSPEC = 0; - AF_INET = 2; - AF_INET6 = 10; + UNSPEC = 0; + INET = 2; + INET6 = 10; } // These are 8 bit unsigned. optional AddressFamily family = 1; diff --git a/src/tcpinfo_c_adapter.c b/src/tcpinfo_c_adapter.c index 3cd9d6e..2d350fa 100644 --- a/src/tcpinfo_c_adapter.c +++ b/src/tcpinfo_c_adapter.c @@ -18,6 +18,7 @@ #include "tcpinfo_c_adapter.h" #include +#include // for INET_DIAG_... #include #include diff --git a/src/tcpinfo_c_adapter.h b/src/tcpinfo_c_adapter.h index d05214d..ac01175 100644 --- a/src/tcpinfo_c_adapter.h +++ b/src/tcpinfo_c_adapter.h @@ -1,3 +1,6 @@ +#ifndef TCPINFO_C_ADAPTER_H_ +#define TCPINFO_C_ADAPTER_H_ + // Copyright 2016 measurement-lab // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -13,24 +16,14 @@ // limitations under the License. /**************************************************************************** -* This file contains declarations that are used by both .c files and .cc -* files. When included in .cc files, it must be enclosed in an extern "C" -* block. -* -* fetch_tcpinfo(...) is implemented in the tcpinfo_c_adapter.c for easy -* access to C libraries. -* update_record(...) is implemented in tcpinfo_lib.cc, because it requires -* access to C++ functions. +* fetch_tcpinfo() is called from C++, but implemented in C to provide clear +* linkage to C library functions. ****************************************************************************/ -#ifndef TCPINFO_C_ADAPTER_H_ -#define TCPINFO_C_ADAPTER_H_ - -#include -#include -#include - -#include "libnetlink.h" +#ifdef __cplusplus +extern "C" { +#endif +#include "libnetlink.h" // For rtnl_filter_t struct inet_diag_arg { int protocol; @@ -40,9 +33,7 @@ struct inet_diag_arg { // `callback` has rtnl_filter_t signature, and is passed to rtnl_dump_filter. int fetch_tcpinfo(rtnl_filter_t callback); -// rtnl_filter_t function to handle each result, passed into fetch_tcpinfo() -// function. Implementation in tcpinfo_lib.cc -int update_record(const struct sockaddr_nl *addr, - struct nlmsghdr *nlh, void *arg); - +#ifdef __cplusplus +} +#endif #endif // TCPINFO_C_ADAPTER_H_ diff --git a/src/tcpinfo_lib.cc b/src/tcpinfo_lib.cc index 41ab714..a25d99f 100644 --- a/src/tcpinfo_lib.cc +++ b/src/tcpinfo_lib.cc @@ -18,6 +18,7 @@ #include #include "connection_cache.h" // For ConnectionTracker +#include "tcpinfo_c_adapter.h" // For update_record, inet_diag_arg extern "C" { #include @@ -25,7 +26,6 @@ extern "C" { #include #include "libnetlink.h" -#include "tcpinfo_c_adapter.h" // declaration of update_record. } namespace mlab { @@ -56,7 +56,7 @@ InetDiagMsgProto::AddressFamily GetFamily(struct inet_diag_msg* r) { auto family = r->idiag_family; if (!InetDiagMsgProto_AddressFamily_IsValid(family)) { fprintf(stderr, "Invalid family: %d\n", family); - return InetDiagMsgProto_AddressFamily_AF_UNSPEC; + return InetDiagMsgProto_AddressFamily_UNSPEC; } else { return InetDiagMsgProto::AddressFamily(family); } @@ -81,15 +81,15 @@ void ParseInetDiagMsg(struct inet_diag_msg* r, InetDiagMsgProto* proto) { dest->set_port(ntohs(r->id.idiag_dport)); switch (proto->family()) { - case InetDiagMsgProto_AddressFamily_AF_INET: + case InetDiagMsgProto_AddressFamily_INET: src->set_ip(r->id.idiag_src, 4); dest->set_ip(r->id.idiag_dst, 4); break; - case InetDiagMsgProto_AddressFamily_AF_INET6: + case InetDiagMsgProto_AddressFamily_INET6: src->set_ip(r->id.idiag_src, 16); dest->set_ip(r->id.idiag_dst, 16); break; - case InetDiagMsgProto_AddressFamily_AF_UNSPEC: + case InetDiagMsgProto_AddressFamily_UNSPEC: // We don't know how to interpret the addresses, so leave them unset. // TODO(gfr) Log a warning here. break; diff --git a/src/tcpinfo_lib.h b/src/tcpinfo_lib.h index 8933453..4698215 100644 --- a/src/tcpinfo_lib.h +++ b/src/tcpinfo_lib.h @@ -36,6 +36,12 @@ extern "C" { namespace mlab { namespace netlink { +extern "C" +// rtnl_filter_t function to handle each result, passed into fetch_tcpinfo() +// function. +int update_record(const struct sockaddr_nl *addr, + struct nlmsghdr *nlh, void *arg); + class TCPInfoPoller; namespace test { // Test support function that required friend status. diff --git a/src/tcpinfo_lib_test.cc b/src/tcpinfo_lib_test.cc index 99fc558..e68d31d 100644 --- a/src/tcpinfo_lib_test.cc +++ b/src/tcpinfo_lib_test.cc @@ -12,20 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include - -#include "gtest/gtest.h" -#include "connection_cache.h" -// TODO(gfr) The include order matters. If tcpinfo_lib.h is included later, -// we get obscure compiler errors processing tcpinfo.pb.h. #include "tcpinfo_lib.h" -extern "C" { -#include // IPPROTO_TCP -#include // AF_INET* +#include +#include "gtest/gtest.h" #include "tcpinfo_c_adapter.h" -} extern mlab::netlink::TCPInfoPoller g_poller_; diff --git a/src/tcpinfo_proto_test.cc b/src/tcpinfo_proto_test.cc index 5c500dc..078c8ff 100644 --- a/src/tcpinfo_proto_test.cc +++ b/src/tcpinfo_proto_test.cc @@ -403,7 +403,7 @@ std::string raw19( TEST(Parser, IPToString) { InetDiagMsgProto p4; - p4.set_family(InetDiagMsgProto_AddressFamily_AF_INET); + p4.set_family(InetDiagMsgProto_AddressFamily_INET); auto* sock_id = p4.mutable_sock_id(); auto* src = sock_id->mutable_source(); src->set_port(1234); @@ -411,7 +411,7 @@ TEST(Parser, IPToString) { EXPECT_EQ(ToString(p4.sock_id().source()), "97.98.99.100:1234"); InetDiagMsgProto p6; - p6.set_family(InetDiagMsgProto_AddressFamily_AF_INET6); + p6.set_family(InetDiagMsgProto_AddressFamily_INET6); auto* sock_id6 = p6.mutable_sock_id(); auto* src6 = sock_id6->mutable_source(); src6->set_port(5678);