feature/echo branch

[isislovecruft]

This is a copy of the echo test which was specified in the bridge reachability testing ticket, and this branch has been cleaned up to remove unrelated commits. It includes some necessary bug fixes for txscapy. I ended up rewriting the test to not use the scapy test template because the template was broken in some cases, and attempting to fix all the scapy related tests and fix the template at the same time was obnoxious; it seemed a better idea to have tests which definitely work correctly and then make sure that any changes to the template do not break any of the tests.

It's been long enough now that I no longer remember what the problems with the template were, though there is a chance I have records of it in my testing logs.

[hellais]

There are a few bugs in this branch. I tried fixing some of them in this branch here: https://github.com/hellais/ooni-probe/tree/feature-echo

Though I am stuck on this as I am not sure exactly what resp is supposed to be:

Traceback (most recent call last):
  File "/root/ooni-probe/ooni/utils/txscapy.py", line 202, in packetReceived
    self.stopSending()
  File "/root/ooni-probe/ooni/utils/txscapy.py", line 214, in stopSending
    self.d.callback(result)
  File "/home/ooni/.virtualenvs/ooni/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 368, in callback
    self._startRunCallbacks(result)
  File "/home/ooni/.virtualenvs/ooni/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 464, in _startRunCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/home/ooni/.virtualenvs/ooni/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 551, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "nettests/experimental/bridge_reachability/echo.py", line 129, in process_answered
    log.msg("Received echo-reply:\n%s" % resp.summary())
exceptions.AttributeError: 'list' object has no attribute 'summary'

[hellais]

The bug fixes in this branch look good, the only problem is with the test.

In the branch the changes that were made are:

1. Replacing log.warn with log.err (is there some code that is missing because such function is not defined)
2. Moving it to use the scapy test template

@isislovecruft was telling me that she was encountering some issues with running the scapy test template. If you could file some bugs on that it would be very helpful.

Also be careful of calling blocking functions (like the scapy sr1) inside of the event loop. It is not sufficient to use maybeDeferred as that is meant to be used when function that don't wait on file descriptors may or may not return a deferred.

[ioerror]

It would be helpful if there was a summary of what this branch is supposed to do/impact/etc; it is very hard to even know if i want to pull down the branch, test it, review it, etc.

In theory, I think d613ca0 is an icmp echo test - eg: a ping variant. However, it is totally unclear until I start the review, if I want to review the code.

So what does this do?

[isislovecruft]

@hellais re: issues:

1. I will put log.warn() and log.fail in a separate branch, named fix/missing-log-functions; will that work okay for you? I'd like to keep things separate.
2. I will also post another separate branch called broken/echo-scapy-template with the version of the test I have that uses the template, so that everyone else can replicate the bugs that I will be posting as tickets, and participate in debugging if they want to.

@ioerror: Is what documentation and and code comments is for, comrade, no? Dear sweet satan, I don't want to document twice....

EDIT:
@hellais: Oops, I didn't see your first comment. resp is supposed to be a scapy.packet.PacketList (or just a list of packets) which is/are the response(s) to the packets sent. That was not in the documentation, my apologies. :P

[ioerror]

@isislovecruft - There is no meaningful commit message for the group of commits. There should be at least one message that explains the entire set of patches overall. How else will I know if I should review the code? As a result, I reviewed every other patch and then left a comment here last.

[ioerror]

Poke?

[isislovecruft]

This was rebased, retested, and a new pull request was issued. Closing.