Large PR - Adapter to C code, demo polling implementation

[gfr10598]

This one has a lot of content, some of it pretty ugly. If you have suggestions for breaking it out into smaller PRs, happy to do so.
I suspect there will be places where I should improve the comments, since I've been pecking at this for about 6 weeks, so parts of it are just in my subconscious now.

---

This change is 

[coveralls]

Coverage increased (+3.6%) to 90.031% when pulling a447e1c on gfr10598:master into 44d4bac on m-lab:master.

[pboothe]

src/README.md, line 6 at r1 (raw file):

## tcpinfo_lib
... contains the primary interface for interacting with the

you are either leaving me hanging or being too clever

---

Comments from Reviewable

[pboothe]

src/tcpinfo_proto_test.cc, line 534 at r1 (raw file):

using namespace std::placeholders;

#define POLL_WITHOUT_FETCH(poller) \

Couldn't this be a method?

---

Comments from Reviewable

[pboothe]

src/tcpinfo_lib_test.cc, line 20 at r1 (raw file):

#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.

:(

This is pretty bad. How hard is it to fix?

---

Comments from Reviewable

[pboothe]

src/tcpinfo_lib.cc, line 443 at r1 (raw file):

  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-(

:(

Maybe lambdas can help?

---

Comments from Reviewable

[pboothe]

src/tcpinfo_lib.cc, line 443 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

:(

Maybe lambdas can help?

Looking into it more: probably not :(

---

Comments from Reviewable

[pboothe]

src/main.cc, line 70 at r1 (raw file):

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!

This comment doesn't match the code?

---

Comments from Reviewable

[pboothe]

src/main.cc, line 20 at r1 (raw file):

void DumpNlMsg(const std::string& nlmsg) {
  std::ofstream out;
  out.open("nldata", std::ofstream::app | std::ofstream::binary);

We're in agreement that hardcoding filenames is bad, right? And only okay in the context of a demo application?

---

Comments from Reviewable

[pboothe]

src/tcpinfo_c_adapter.c, line 72 at r1 (raw file):

  req.r.sdiag_protocol = protocol;
  req.r.idiag_states = 1 << SS_ESTABLISHED;
  // DO NOT SUBMIT

?

---

Comments from Reviewable

[pboothe]

src/tcpinfo_c_adapter.c, line 46 at r1 (raw file):

};

#define MAGIC_SEQ 123456

Please make this a random number. 123456 is, imo, a worryingly probable input. Or, put another way: https://vine.co/v/hl7Zpzh1979

---

Comments from Reviewable

[pboothe]

Reviewed 1 of 10 files at r1.
Review status: 1 of 10 files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+3.7%) to 90.123% when pulling 3fae7b9 on gfr10598:master into 44d4bac on m-lab:master.

[coveralls]

Coverage increased (+3.7%) to 90.123% when pulling 9645932 on gfr10598:master into 44d4bac on m-lab:master.

[gfr10598]

Review status: 1 of 10 files reviewed at latest revision, 8 unresolved discussions.

---

src/main.cc, line 20 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

We're in agreement that hardcoding filenames is bad, right? And only okay in the context of a demo application?

Yup!!

---

src/main.cc, line 70 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

This comment doesn't match the code?

Done.

---

src/README.md, line 6 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

you are either leaving me hanging or being too clever

Oops. Fixed, and tweaked the others, as well.

---

src/tcpinfo_c_adapter.c, line 46 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Please make this a random number. 123456 is, imo, a worryingly probable input. Or, put another way: https://vine.co/v/hl7Zpzh1979

Done. May want to actually make it a sequence number in future.

---

src/tcpinfo_c_adapter.c, line 72 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

?

Done. I had some changes in another clone that got left behind.

---

src/tcpinfo_lib.cc, line 443 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Looking into it more: probably not :(

It is a big annoyance, but not a show stopper, so leaving it alone for now.

---

src/tcpinfo_lib_test.cc, line 20 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

:(

This is pretty bad. How hard is it to fix?

Don't know. It is better than it used to be, but still unsatisfying. Leaving it as a TODO for now, because I'm not sure what to try next.

---

src/tcpinfo_proto_test.cc, line 534 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Couldn't this be a method?

Done. (with a little gymnastics).

---

Comments from Reviewable

[pboothe]

src/tcpinfo_lib_test.cc, line 20 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Don't know. It is better than it used to be, but still unsatisfying. Leaving it as a TODO for now, because I'm not sure what to try next.

Looks like an extern C problem - all the transitive inclusions of "tcpinfo_c_adapter.h" are externed in the code below, except for the ones already included by "connection_cache.h" and "tcpinfo_lib.h".

---

Comments from Reviewable

[pboothe]

LGTM except for the include-order problem, which if it's not too bad I would strongly prefer fixing. Right now, if I'm right, transitive inclusions are randomly extern C-ified or not.

---

Comments from Reviewable

[pboothe]

Reviewed 1 of 3 files at r2.
Review status: 2 of 10 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[gfr10598]

I've made vast improvements to the extern "C" stuff. Main problem was actually a collision with tcpinfo.pb.h declarations of AF_* colliding with socket.h macros.
Going ahead with submit, but PTAL and let me know if you find any other suggestions.

---

Review status: 2 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[gfr10598]

Review status: 2 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

src/tcpinfo_lib_test.cc, line 20 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Looks like an extern C problem - all the transitive inclusions of "tcpinfo_c_adapter.h" are externed in the code below, except for the ones already included by "connection_cache.h" and "tcpinfo_lib.h".

Fixed.

---

Comments from Reviewable

[coveralls]

Coverage increased (+3.7%) to 90.123% when pulling ae23c27 on gfr10598:master into 44d4bac on m-lab:master.