commit 74a146834ee6609fd4f66c0b5f5381654acf8feb Author: Arturo Filastò art@fuffa.org Date: Thu Jun 19 16:06:50 2014 +0200

Fix bug that lead to some reports not being submitted.

Pep8 related fixes. --- ooni/deck.py | 14 ++++--- ooni/managers.py | 22 ++++++---- ooni/nettest.py | 123 +++++++++++++++++++++++++++++++++--------------------- ooni/oonicli.py | 81 +++++++++++++++++++---------------- ooni/reporter.py | 3 +- 5 files changed, 144 insertions(+), 99 deletions(-)

diff --git a/ooni/deck.py b/ooni/deck.py index ad90d03..f38b098 100644 --- a/ooni/deck.py +++ b/ooni/deck.py @@ -7,10 +7,9 @@ from ooni.utils import log from ooni import errors as e

from twisted.python.filepath import FilePath -from twisted.internet import reactor, defer +from twisted.internet import defer

import os -import re import yaml import json from hashlib import sha256 @@ -187,12 +186,15 @@ class Deck(InputFile): response = yield self.oonibclient.lookupTestHelpers(required_test_helpers)

for net_test_loader in self.netTestLoaders: - log.msg("Setting collector and test helpers for %s" % net_test_loader.testDetails['test_name']) + log.msg("Setting collector and test helpers for %s" % + net_test_loader.testDetails['test_name'])

# Only set the collector if the no collector has been specified # from the command line or via the test deck. - if not required_test_helpers and net_test_loader in requires_collector: - log.msg("Using the default collector: %s" % response['default']['collector']) + if not net_test_loader.requiredTestHelpers and \ + net_test_loader in requires_collector: + log.msg("Using the default collector: %s" % + response['default']['collector']) net_test_loader.collector = response['default']['collector'].encode('utf-8') continue

@@ -223,6 +225,6 @@ class Deck(InputFile): try: input_file.verify() except AssertionError: - raise e.UnableToLoadDeckInput, cached_path + raise e.UnableToLoadDeckInput

i['test_class'].localOptions[i['key']] = input_file.cached_file diff --git a/ooni/managers.py b/ooni/managers.py index d03d2c9..65e4ce1 100644 --- a/ooni/managers.py +++ b/ooni/managers.py @@ -1,9 +1,9 @@ import itertools

-from twisted.internet import defer from ooni.utils import log from ooni.settings import config

+ def makeIterable(item): """ Takes as argument or an iterable and if it's not an iterable object then it @@ -15,6 +15,7 @@ def makeIterable(item): iterable = iter([item]) return iterable

+ class TaskManager(object): retries = 2 concurrency = 10 @@ -60,7 +61,7 @@ class TaskManager(object): self._run(task) except StopIteration: break - except ValueError as exc: + except ValueError: # XXX this is a workaround the race condition that leads the # _tasks generator to throw the exception # ValueError: generator already called. @@ -103,8 +104,8 @@ class TaskManager(object):

def schedule(self, task_or_task_iterator): """ - Takes as argument a single task or a task iterable and appends it to the task - generator queue. + Takes as argument a single task or a task iterable and appends it to + the task generator queue. """ log.debug("Starting this task %s" % repr(task_or_task_iterator))

@@ -136,7 +137,9 @@ class TaskManager(object): """ raise NotImplemented

+ class LinkedTaskManager(TaskManager): + def __init__(self): super(LinkedTaskManager, self).__init__() self.child = None @@ -160,10 +163,13 @@ class LinkedTaskManager(TaskManager): if self.parent: self.parent._fillSlots()

+ class MeasurementManager(LinkedTaskManager): + """ - This is the Measurement Tracker. In here we keep track of active measurements - and issue new measurements once the active ones have been completed. + This is the Measurement Tracker. In here we keep track of active + measurements and issue new measurements once the active ones have been + completed.

MeasurementTracker does not keep track of the typology of measurements that it is running. It just considers a measurement something that has an input @@ -172,6 +178,7 @@ class MeasurementManager(LinkedTaskManager): NetTest on the contrary is aware of the typology of measurements that it is dispatching as they are logically grouped by test file. """ + def __init__(self): if config.advanced.measurement_retries: self.retries = config.advanced.measurement_retries @@ -186,7 +193,9 @@ class MeasurementManager(LinkedTaskManager): def failed(self, failure, measurement): pass

+ class ReportEntryManager(LinkedTaskManager): + def __init__(self): if config.advanced.reporting_retries: self.retries = config.advanced.reporting_retries @@ -200,4 +209,3 @@ class ReportEntryManager(LinkedTaskManager):

def failed(self, failure, task): pass - diff --git a/ooni/nettest.py b/ooni/nettest.py index 166a4f9..1547617 100644 --- a/ooni/nettest.py +++ b/ooni/nettest.py @@ -16,9 +16,11 @@ from ooni import errors as e from inspect import getmembers from StringIO import StringIO

+ class NoTestCasesFound(Exception): pass

+ def get_test_methods(item, method_prefix="test_"): """ Look for test_ methods in subclasses of NetTestCase @@ -36,6 +38,7 @@ def get_test_methods(item, method_prefix="test_"): pass return test_cases

+ def getTestClassFromFile(net_test_file): """ Will return the first class that is an instance of NetTestCase. @@ -51,10 +54,12 @@ def getTestClassFromFile(net_test_file): except (TypeError, AssertionError): pass

+ def getOption(opt_parameter, required_options, type='text'): """ Arguments: - usage_options: a list as should be the optParameters of an UsageOptions class. + usage_options: a list as should be the optParameters of an UsageOptions + class.

required_options: a list containing the strings of the options that are required. @@ -77,16 +82,19 @@ def getOption(opt_parameter, required_options, type='text'): required = False

return {'description': description, - 'value': default, 'required': required, - 'type': type - } + 'value': default, 'required': required, + 'type': type + } +

def getArguments(test_class): arguments = {} if test_class.inputFile: option_name = test_class.inputFile[0] - arguments[option_name] = getOption(test_class.inputFile, - test_class.requiredOptions, type='file') + arguments[option_name] = getOption( + test_class.inputFile, + test_class.requiredOptions, + type='file') try: list(test_class.usageOptions.optParameters) except AttributeError: @@ -94,16 +102,20 @@ def getArguments(test_class):

for opt_parameter in test_class.usageOptions.optParameters: option_name = opt_parameter[0] - opt_type="text" + opt_type = "text" if opt_parameter[3].lower().startswith("file"): - opt_type="file" - arguments[option_name] = getOption(opt_parameter, - test_class.requiredOptions, type=opt_type) + opt_type = "file" + arguments[option_name] = getOption( + opt_parameter, + test_class.requiredOptions, + type=opt_type)

return arguments

+ def test_class_name_to_name(test_class_name): - return test_class_name.lower().replace(' ','_') + return test_class_name.lower().replace(' ', '_') +

def getNetTestInformation(net_test_file): """ @@ -122,21 +134,23 @@ def getNetTestInformation(net_test_file):

test_id = os.path.basename(net_test_file).replace('.py', '') information = {'id': test_id, - 'name': test_class.name, - 'description': test_class.description, - 'version': test_class.version, - 'arguments': getArguments(test_class), - 'path': net_test_file, - } + 'name': test_class.name, + 'description': test_class.description, + 'version': test_class.version, + 'arguments': getArguments(test_class), + 'path': net_test_file, + } return information

+ class NetTestLoader(object): method_prefix = 'test' collector = None requiresTor = False

def __init__(self, options, test_file=None, test_string=None): - self.onionInputRegex = re.compile("(httpo://[a-z0-9]{16}.onion)/input/([a-z0-9]{64})$") + self.onionInputRegex = re.compile( + "(httpo://[a-z0-9]{16}.onion)/input/([a-z0-9]{64})$") self.options = options self.testCases = []

@@ -204,20 +218,19 @@ class NetTestLoader(object): input_file_hashes.append(input_file['hash'])

test_details = {'start_time': time.time(), - 'probe_asn': config.probe_ip.geodata['asn'], - 'probe_cc': config.probe_ip.geodata['countrycode'], - 'probe_ip': config.probe_ip.geodata['ip'], - 'probe_city': config.probe_ip.geodata['city'], - 'test_name': self.testName, - 'test_version': self.testVersion, - 'software_name': 'ooniprobe', - 'software_version': software_version, - 'options': self.options, - 'input_hashes': input_file_hashes - } + 'probe_asn': config.probe_ip.geodata['asn'], + 'probe_cc': config.probe_ip.geodata['countrycode'], + 'probe_ip': config.probe_ip.geodata['ip'], + 'probe_city': config.probe_ip.geodata['city'], + 'test_name': self.testName, + 'test_version': self.testVersion, + 'software_name': 'ooniprobe', + 'software_version': software_version, + 'options': self.options, + 'input_hashes': input_file_hashes + } return test_details

- def _parseNetTestOptions(self, klass): """ Helper method to assemble the options into a single UsageOptions object @@ -360,7 +373,9 @@ class NetTestLoader(object): pass return test_cases

+ class NetTestState(object): + def __init__(self, allTasksDone): """ This keeps track of the state of a running NetTests case. @@ -407,6 +422,7 @@ class NetTestState(object): self.completedScheduling = True self.checkAllTasksDone()

+ class NetTest(object): director = None

@@ -480,15 +496,17 @@ class NetTest(object):

if self.director: measurement.done.addCallback(self.director.measurementSucceeded, - measurement) + measurement) measurement.done.addErrback(self.director.measurementFailed, - measurement) + measurement) return measurement

@defer.inlineCallbacks def initializeInputProcessor(self): for test_class, _ in self.testCases: - test_class.inputs = yield defer.maybeDeferred(test_class().getInputProcessor) + test_class.inputs = yield defer.maybeDeferred( + test_class().getInputProcessor + ) if not test_class.inputs: test_class.inputs = [None]

@@ -506,7 +524,10 @@ class NetTest(object): test_instance.summary = self.summary for method in test_methods: log.debug("Running %s %s" % (test_class, method)) - measurement = self.makeMeasurement(test_instance, method, input) + measurement = self.makeMeasurement( + test_instance, + method, + input) measurements.append(measurement.done) self.state.taskCreated() yield measurement @@ -519,6 +540,7 @@ class NetTest(object): # Call the postProcessor, which must return a single report # or a deferred post.addCallback(test_instance.postProcessor) + def noPostProcessor(failure, report): failure.trap(e.NoPostProcessor) return report @@ -526,30 +548,32 @@ class NetTest(object): post.addCallback(self.report.write)

if self.report and self.director: - #ghetto hax to keep NetTestState counts are accurate + # ghetto hax to keep NetTestState counts are accurate [post.addBoth(self.doneReport) for _ in measurements]

self.state.allTasksScheduled()

+ class NetTestCase(object): + """ This is the base of the OONI nettest universe. When you write a nettest you will subclass this object.

* inputs: can be set to a static set of inputs. All the tests (the methods - starting with the "test" prefix) will be run once per input. At every run - the _input_ attribute of the TestCase instance will be set to the value of - the current iteration over inputs. Any python iterable object can be set - to inputs. + starting with the "test" prefix) will be run once per input. At every + run the _input_ attribute of the TestCase instance will be set to the + value of the current iteration over inputs. Any python iterable object + can be set to inputs.

- * inputFile: attribute should be set to an array containing the command line - argument that should be used as the input file. Such array looks like - this: + * inputFile: attribute should be set to an array containing the command + line argument that should be used as the input file. Such array looks + like this:

``["commandlinearg", "c", "default value" "The description"]``

- The second value of such arrray is the shorthand for the command line arg. - The user will then be able to specify inputs to the test via: + The second value of such arrray is the shorthand for the command line + arg. The user will then be able to specify inputs to the test via:

``ooniprobe mytest.py --commandlinearg path/to/file.txt``

@@ -573,12 +597,14 @@ class NetTestCase(object):

* requiresRoot: set to True if the test must be run as root.

- * usageOptions: a subclass of twisted.python.usage.Options for processing of command line arguments + * usageOptions: a subclass of twisted.python.usage.Options for processing + of command line arguments

* localOptions: contains the parsed command line arguments.

Quirks: - Every class that is prefixed with test *must* return a twisted.internet.defer.Deferred. + Every class that is prefixed with test *must* return a + twisted.internet.defer.Deferred. """ name = "This test is nameless" author = "Jane Doe foo@example.com" @@ -603,6 +629,7 @@ class NetTestCase(object): requiresTor = False

localOptions = {} + def _setUp(self): """ This is the internal setup method to be overwritten by templates. @@ -734,8 +761,8 @@ class NetTestCase(object): for required_option in self.requiredOptions: log.debug("Checking if %s is present" % required_option) if required_option not in self.localOptions or \ - self.localOptions[required_option] == None: - missing_options.append(required_option) + self.localOptions[required_option] is None: + missing_options.append(required_option) if missing_options: raise e.MissingRequiredOption(missing_options)

diff --git a/ooni/oonicli.py b/ooni/oonicli.py index cc23ec5..f84ac2d 100644 --- a/ooni/oonicli.py +++ b/ooni/oonicli.py @@ -1,5 +1,3 @@ -#-*- coding: utf-8 -*- - import sys import os import yaml @@ -17,6 +15,7 @@ from ooni.nettest import NetTestLoader

from ooni.utils import log, checkForRoot

+ class Options(usage.Options): synopsis = """%s [options] [path to test].py """ % (os.path.basename(sys.argv[0]),) @@ -34,27 +33,27 @@ class Options(usage.Options): ["verbose", "v"] ]

- optParameters = [["reportfile", "o", None, "report file name"], - ["testdeck", "i", None, - "Specify as input a test deck: a yaml file containing the tests to run and their arguments"], - ["collector", "c", None, - "Address of the collector of test results. This option should not be used, but you should always use a bouncer."], - ["bouncer", "b", 'httpo://nkvphnp3p6agi5qq.onion', - "Address of the bouncer for test helpers. default: httpo://nkvphnp3p6agi5qq.onion"], - ["logfile", "l", None, "log file name"], - ["pcapfile", "O", None, "pcap file name"], - ["configfile", "f", None, - "Specify a path to the ooniprobe configuration file"], - ["datadir", "d", None, - "Specify a path to the ooniprobe data directory"], - ["annotations", "a", None, - "Annotate the report with a key:value[, key:value] format."] - ] + optParameters = [ + ["reportfile", "o", None, "report file name"], + ["testdeck", "i", None, + "Specify as input a test deck: a yaml file containing the tests to run and their arguments"], + ["collector", "c", None, + "Address of the collector of test results. This option should not be used, but you should always use a bouncer."], + ["bouncer", "b", 'httpo://nkvphnp3p6agi5qq.onion', + "Address of the bouncer for test helpers. default: httpo://nkvphnp3p6agi5qq.onion"], + ["logfile", "l", None, "log file name"], + ["pcapfile", "O", None, "pcap file name"], + ["configfile", "f", None, + "Specify a path to the ooniprobe configuration file"], + ["datadir", "d", None, + "Specify a path to the ooniprobe data directory"], + ["annotations", "a", None, + "Annotate the report with a key:value[, key:value] format."]]

compData = usage.Completions( extraActions=[usage.CompleteFiles( - "*.py", descr="file | module | package | TestCase | testMethod", - repeat=True)],) + "*.py", descr="file | module | package | TestCase | testMethod", + repeat=True)],)

tracer = None

@@ -85,6 +84,7 @@ class Options(usage.Options): except: raise usage.UsageError("No test filename specified!")

+ def parseOptions(): print "WARNING: running ooniprobe involves some risk that varies greatly" print " from country to country. You should be aware of this when" @@ -94,12 +94,13 @@ def parseOptions(): cmd_line_options.getUsage() try: cmd_line_options.parseOptions() - except usage.UsageError, ue: + except usage.UsageError as ue: print cmd_line_options.getUsage() - raise SystemExit, "%s: %s" % (sys.argv[0], ue) + raise SystemExit("%s: %s" % (sys.argv[0], ue))

return dict(cmd_line_options)

+ def runWithDirector(logging=True, start_tor=True): """ Instance the director, parse command line options and start an ooniprobe @@ -122,9 +123,9 @@ def runWithDirector(logging=True, start_tor=True): try: checkForRoot() except errors.InsufficientPrivileges: - log.err("Insufficient Privileges to capture packets." - " See ooniprobe.conf privacy.includepcap") - sys.exit(2) + log.err("Insufficient Privileges to capture packets." + " See ooniprobe.conf privacy.includepcap") + sys.exit(2)

director = Director() if global_options['list']: @@ -157,10 +158,10 @@ def runWithDirector(logging=True, start_tor=True): sys.exit(1) global_options["annotations"] = annotations

- #XXX: This should mean no bouncer either! + # XXX: This should mean no bouncer either! if global_options['no-collector']: log.msg("Not reporting using a collector") - collector = global_options['collector'] = None + global_options['collector'] = None global_options['bouncer'] = None

deck = Deck() @@ -178,16 +179,16 @@ def runWithDirector(logging=True, start_tor=True): log.debug("No test deck detected") test_file = nettest_to_path(global_options['test_file'], True) net_test_loader = NetTestLoader(global_options['subargs'], - test_file=test_file) + test_file=test_file) deck.insert(net_test_loader) - except errors.MissingRequiredOption, option_name: + except errors.MissingRequiredOption as option_name: log.err('Missing required option: "%s"' % option_name) print net_test_loader.usageOptions().getUsage() sys.exit(2) - except errors.NetTestNotFound, path: + except errors.NetTestNotFound as path: log.err('Requested NetTest file not found (%s)' % path) sys.exit(3) - except usage.UsageError, e: + except usage.UsageError as e: log.err(e) print net_test_loader.usageOptions().getUsage() sys.exit(4) @@ -217,24 +218,29 @@ def runWithDirector(logging=True, start_tor=True): log.err("Tor does not appear to be running") log.err("Reporting with the collector %s is not possible" % global_options['collector']) - log.msg("Try with a different collector or disable collector reporting with -n") + log.msg( + "Try with a different collector or disable collector reporting with -n")

elif isinstance(failure.value, errors.InvalidOONIBCollectorAddress): log.err("Invalid format for oonib collector address.") - log.msg("Should be in the format http://<collector_address>:<port>") + log.msg( + "Should be in the format http://<collector_address>:<port>") log.msg("for example: ooniprobe -c httpo://nkvphnp3p6agi5qq.onion")

elif isinstance(failure.value, errors.UnableToLoadDeckInput): log.err("Unable to fetch the required inputs for the test deck.") - log.msg("Please file a ticket on our issue tracker: https://github.com/thetorproject/ooni-probe/issues") + log.msg( + "Please file a ticket on our issue tracker: https://github.com/thetorproject/ooni-probe/issues")

elif isinstance(failure.value, errors.CouldNotFindTestHelper): log.err("Unable to obtain the required test helpers.") - log.msg("Try with a different bouncer or check that Tor is running properly.") + log.msg( + "Try with a different bouncer or check that Tor is running properly.")

elif isinstance(failure.value, errors.CouldNotFindTestCollector): log.err("Could not find a valid collector.") - log.msg("Try with a different bouncer, specify a collector with -c or disable reporting to a collector with -n.") + log.msg( + "Try with a different bouncer, specify a collector with -c or disable reporting to a collector with -n.")

elif isinstance(failure.value, errors.ProbeIPUnknown): log.err("Failed to lookup probe IP address.") @@ -267,7 +273,8 @@ def runWithDirector(logging=True, start_tor=True): if not global_options['no-collector']: if global_options['collector']: collector = global_options['collector'] - elif 'collector' in config.reports and config.reports['collector']: + elif 'collector' in config.reports \ + and config.reports['collector']: collector = config.reports['collector'] elif net_test_loader.collector: collector = net_test_loader.collector diff --git a/ooni/reporter.py b/ooni/reporter.py index 60df7eb..8341b1a 100644 --- a/ooni/reporter.py +++ b/ooni/reporter.py @@ -546,7 +546,8 @@ class Report(object): def open_oonib_reporter(self): def creation_failed(failure): self.oonib_reporter = None - return self.report_log.creation_failed(self.report_filename) + return self.report_log.creation_failed(self.report_filename, + self.collector_address)

def created(report_id): return self.report_log.created(self.report_filename,