bwtester: simplification, cleanup

[matzf]

Simplification, refactoring, cleanup of the bwtester codebase.

This does not introduce any (significant) behaviour changes; the
applications are compatible with the version before this change.
One breaking change is that we've removed the rather pointless -id and
-log_dir flags from the bwtestserver.
In the client, the output format has been slightly adapted.

The motivation for this is to make it easier to convert bwtester to use
the pan library. In particular, one goal was to make HandleDCCnnSend/Receive
functions (shared between client and server) dumber, i.e making them do
nothing but Write/Read on the connection. This will help to adapt this to
pan with its asymmetric dialed/listening connections.

The most relevant changes are:

• split functions, reduce duplicate code
• try to get some semblance of reasonable error handling
• simplify the termination of tests by simplifying; set Read/Write
  deadlines for the data channel (once!), ensuring that the reader/write
  routines abort in time.
• remove ExpectedFinishTime from BwtestResults struct. Was only used
  for (unnecessarily confusing) internal bookkeeping, does not need to
  be on the wire. The "gob" encoding on the wire is still compatible.
• server: avoid excessive concurrent access to results map to get rid of
  the excessive locking code. Only add the result entry once it's
  actually available (it's so much easier...).
• client: shorten output format for Interarrival time (~analogous to
  ping output)

Note that this is a limited cleanup pass, trying not to change too much
at once and not to introduce breaking changes. The code (and the
functionality) can clearly still be improved...

---

This change is 

[juagargi]

Thanks for maintaining this bwtest toolset tidy.
We would like to use these tools and supersede the ad-hoc cbrtester. One of the useful things to do with cbrtester is to leave it running in the background, and start loading the BR.
Some things that would make this tool supersede cbrtester:

• Instant reporting. Configure reporting every N seconds.
• Allow noop for one of the directions: typically client-to-server would be empty to test load specific border routers. At the moment the reporting will crash with a divide by zero.
• Infinite duration. At the moment we could write a big number of seconds for the duration parameter if we change the value of MaxDuration, so this is no big issue.
• Fine tune reporting: report delays over certain threshold. This is useful while debugging a BR.
• Useful to identify packets in the captures is the ability of setting up certain pattern in the payload. This greatly simplified finding where the delays were being added.
• Not present in cbrtester but definitely useful: select the path with a policy (filter predicate).

It could be nice to have a symlink to the binary with a different name, check argv[0] and prepend a default set of arguments if invoked like e.g. cbrbwclienttest.

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

[matzf]

Allow noop for one of the directions: typically client-to-server would be empty to test load specific border routers. At the moment the reporting will crash with a divide by zero.

Right, I had this fixed already on a different branch, forgot to put this here. Fixed now.
Note that this is a minimal fix only, there are still a number of lurking div-by-zero issues with when parsing the parameters. The easy case of providing only the bandwidth (e.g. -sc 0 -cs 10M) works fine though.

• Infinite duration. At the moment we could write a big number of seconds for the duration parameter if we change the value of MaxDuration, so this is no big issue.

I agree, 10s is rather short. I guess it was chosen short that to ensure some "fairness" in case multiple clients want to connect and as a simple way to ensure that tests end even if clients crash. We could increase MaxDuration to 5 minutes or so?

• Useful to identify packets in the captures is the ability of setting up certain pattern in the payload. This greatly simplified finding where the delays were being added.

Sure, this can be done. Currently, both sides expect the encrypted block number, so this would be a protocol change. That's fine, but I'd rather look at this separately later (trying to separate refactoring from protocol change).

• Instant reporting. Configure reporting every N seconds.
• Fine tune reporting: report delays over certain threshold. This is useful while debugging a BR.

This can done, probably also requires a protocol change (and a change of the output format, obviously).

Reviewable status: 1 of 4 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

[juagargi]

Issue #211 covers the currently missing features for bwtester to supersede cbrtester

Reviewed all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained