commit 31456707624847f8b0404b2e84ebcc2dc7fe22fd Author: Damian Johnson <atagar@torproject.org> Date: Wed May 29 08:19:54 2013 -0700 System module's call() function ignored exit statuses Well... oops. Our call() method was documented as raising an OSError (or returning the default value) when the call fails but it completely ignored the exit status. In practice the function only behaved as documented when Popen failed - otherwise it returned an empty list. Fixing this behavior and updating most of our callers since they expected call() to not raise any exceptions. --- docs/change_log.rst | 3 ++ run_tests.py | 2 +- stem/descriptor/__init__.py | 1 + stem/util/system.py | 55 +++++++++++++++++++++--------------- test/integ/util/system.py | 2 +- test/settings.cfg | 2 +- test/unit/response/protocolinfo.py | 2 +- test/unit/util/system.py | 10 +++++-- test/util.py | 10 +++++-- 9 files changed, 56 insertions(+), 31 deletions(-) diff --git a/docs/change_log.rst b/docs/change_log.rst index cb0134e..9d49537 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -54,6 +54,9 @@ The following are only available within stem's `git repository * :func:`~stem.util.system.set_process_name` inserted spaces between characters (:trac:`8631`) * :func:`~stem.util.system.get_pid_by_name` can now pull for all processes with a given name + * :func:`~stem.util.system.call` ignored the subprocess' exit status + * Added :func:`stem.util.system.get_start_time` + * Added :func:`stem.util.system.get_bsd_jail_path` * **Website** diff --git a/run_tests.py b/run_tests.py index 67c9a1d..6f77b0f 100755 --- a/run_tests.py +++ b/run_tests.py @@ -37,7 +37,7 @@ from test.util import STEM_BASE, Target, Task # We do an integration test run for each run target we get. # # * Attribute Target (like CHROOT and ONLINE) which indicates -# non-configuration changes to ur test runs. These are applied to all +# non-configuration changes to your test runs. These are applied to all # integration runs that we perform. ARGS = { diff --git a/stem/descriptor/__init__.py b/stem/descriptor/__init__.py index 2567a4b..7252e6a 100644 --- a/stem/descriptor/__init__.py +++ b/stem/descriptor/__init__.py @@ -14,6 +14,7 @@ Package for parsing and processing descriptor data. Descriptor - Common parent for all descriptor file types. |- get_path - location of the descriptor on disk if it came from a file |- get_archive_path - location of the descriptor within the archive it came from + |- get_bytes - similar to str(), but provides our original bytes content |- get_unrecognized_lines - unparsed descriptor content +- __str__ - string that the descriptor was made from diff --git a/stem/util/system.py b/stem/util/system.py index 2f3cbd2..ea8513f 100644 --- a/stem/util/system.py +++ b/stem/util/system.py @@ -210,9 +210,10 @@ def is_running(command): primary_resolver = IS_RUNNING_PS_LINUX secondary_resolver = IS_RUNNING_PS_BSD - command_listing = call(primary_resolver) + command_listing = call(primary_resolver, None) + if not command_listing: - command_listing = call(secondary_resolver) + command_listing = call(secondary_resolver, None) if command_listing: command_listing = map(unicode.strip, command_listing) @@ -253,7 +254,7 @@ def get_pid_by_name(process_name, multiple = False): # 3392 if is_available("pgrep"): - results = call(GET_PID_BY_NAME_PGREP % process_name) + results = call(GET_PID_BY_NAME_PGREP % process_name, None) if results: try: @@ -274,7 +275,7 @@ def get_pid_by_name(process_name, multiple = False): # 3392 3283 if is_available("pidof"): - results = call(GET_PID_BY_NAME_PIDOF % process_name) + results = call(GET_PID_BY_NAME_PIDOF % process_name, None) if results and len(results) == 1: try: @@ -306,7 +307,7 @@ def get_pid_by_name(process_name, multiple = False): if is_available("ps"): if not is_bsd(): # linux variant of ps - results = call(GET_PID_BY_NAME_PS_LINUX % process_name) + results = call(GET_PID_BY_NAME_PS_LINUX % process_name, None) if results: try: @@ -321,7 +322,7 @@ def get_pid_by_name(process_name, multiple = False): if is_bsd(): # bsd variant of ps - results = call(GET_PID_BY_NAME_PS_BSD) + results = call(GET_PID_BY_NAME_PS_BSD, None) if results: # filters results to those with our process name @@ -352,7 +353,7 @@ def get_pid_by_name(process_name, multiple = False): # 2561 if is_available("lsof"): - results = call(GET_PID_BY_NAME_LSOF % process_name) + results = call(GET_PID_BY_NAME_LSOF % process_name, None) if results: try: @@ -409,7 +410,7 @@ def get_pid_by_port(port): # udp6 0 0 fe80::7ae4:ff:fe2f::123 :::* - if is_available("netstat"): - results = call(GET_PID_BY_PORT_NETSTAT) + results = call(GET_PID_BY_PORT_NETSTAT, None) if results: # filters to results with our port @@ -442,7 +443,7 @@ def get_pid_by_port(port): # _tor tor 4397 20 tcp4 51.64.7.84:51946 32.83.7.104:443 if is_available("sockstat"): - results = call(GET_PID_BY_PORT_SOCKSTAT % port) + results = call(GET_PID_BY_PORT_SOCKSTAT % port, None) if results: # filters to results where this is the local port @@ -474,7 +475,7 @@ def get_pid_by_port(port): # tor 1745 atagar 6u IPv4 14229 0t0 TCP 127.0.0.1:9051 (LISTEN) if is_available("lsof"): - results = call(GET_PID_BY_PORT_LSOF) + results = call(GET_PID_BY_PORT_LSOF, None) if results: # filters to results with our port @@ -516,9 +517,9 @@ def get_pid_by_open_file(path): # 4762 if is_available("lsof"): - results = call(GET_PID_BY_FILE_LSOF % path) + results = call(GET_PID_BY_FILE_LSOF % path, []) - if results and len(results) == 1: + if len(results) == 1: pid = results[0].strip() if pid.isdigit(): @@ -552,7 +553,7 @@ def get_cwd(pid): # 3799: /home/atagar # 5839: No such process - results = call(GET_CWD_PWDX % pid) + results = call(GET_CWD_PWDX % pid, None) if not results: log.debug("%s pwdx didn't return any results" % logging_prefix) @@ -579,9 +580,9 @@ def get_cwd(pid): # n/Users/atagar/tor/src/or if is_available("lsof"): - results = call(GET_CWD_LSOF % pid) + results = call(GET_CWD_LSOF % pid, []) - if results and len(results) == 2 and results[1].startswith("n/"): + if len(results) == 2 and results[1].startswith("n/"): lsof_result = results[1][1:].strip() # If we lack read permissions for the cwd then it returns... @@ -613,9 +614,9 @@ def get_start_time(pid): pass try: - ps_results = stem.util.system.call("ps -p %s -o etime" % pid) + ps_results = call("ps -p %s -o etime" % pid, []) - if ps_results and len(ps_results) >= 2: + if len(ps_results) >= 2: etime = ps_results[1].strip() return time.time() - stem.util.str_tools.parse_short_time_label(etime) except: @@ -623,6 +624,7 @@ def get_start_time(pid): return None + def get_bsd_jail_id(pid): """ Gets the jail id for a process. These seem to only exist for FreeBSD (this @@ -641,9 +643,9 @@ def get_bsd_jail_id(pid): # JID # 1 - ps_output = call(GET_BSD_JAIL_ID_PS % pid) + ps_output = call(GET_BSD_JAIL_ID_PS % pid, []) - if ps_output and len(ps_output) == 2 and len(ps_output[1].split()) == 1: + if len(ps_output) == 2 and len(ps_output[1].split()) == 1: jid = ps_output[1].strip() if jid.isdigit(): @@ -671,7 +673,7 @@ def get_bsd_jail_path(jid): # Output should be something like: # JID IP Address Hostname Path # 1 10.0.0.2 tor-jail /usr/jails/tor-jail - + jls_output = call(GET_BSD_JAIL_PATH % jid, []) if len(jls_output) == 2 and len(jls_output[1].split()) == 4: @@ -725,7 +727,7 @@ def expand_path(path, cwd = None): return relative_path -def call(command, default = UNDEFINED): +def call(command, default = UNDEFINED, ignore_exit_status = False): """ Issues a command in a subprocess, blocking until completion and returning the results. This is not actually ran in a shell so pipes and other shell syntax @@ -733,6 +735,8 @@ def call(command, default = UNDEFINED): :param str command: command to be issued :param object default: response if the query fails + :param bool ignore_exit_status: reports failure if our command's exit status + was non-zero :returns: **list** with the lines of output from the command @@ -743,7 +747,9 @@ def call(command, default = UNDEFINED): is_shell_command = command.split(" ")[0] in SHELL_COMMANDS start_time = time.time() - stdout, stderr = subprocess.Popen(command.split(), stdout = subprocess.PIPE, stderr = subprocess.PIPE, shell = is_shell_command).communicate() + process = subprocess.Popen(command.split(), stdout = subprocess.PIPE, stderr = subprocess.PIPE, shell = is_shell_command) + + stdout, stderr = process.communicate() stdout, stderr = stdout.strip(), stderr.strip() runtime = time.time() - start_time @@ -757,6 +763,11 @@ def call(command, default = UNDEFINED): elif stderr: log.trace(trace_prefix + ", stderr:\n%s" % stderr) + exit_code = process.poll() + + if not ignore_exit_status and exit_code != 0: + raise OSError("%s returned exit status %i" % (command, exit_code)) + if stdout: return stdout.decode("utf-8", "replace").splitlines() else: diff --git a/test/integ/util/system.py b/test/integ/util/system.py index 1392dfd..0bf77ac 100644 --- a/test/integ/util/system.py +++ b/test/integ/util/system.py @@ -21,7 +21,7 @@ def filter_system_call(prefixes): function if it matches one of the prefixes, and acts as a no-op otherwise. """ - def _filter_system_call(command): + def _filter_system_call(command, default): for prefix in prefixes: if command.startswith(prefix): real_call_function = mocking.get_real_function(stem.util.system.call) diff --git a/test/settings.cfg b/test/settings.cfg index bd3e3cd..db318e4 100644 --- a/test/settings.cfg +++ b/test/settings.cfg @@ -132,7 +132,7 @@ pep8.ignore E127 pyflakes.ignore stem/prereq.py => 'RSA' imported but unused pyflakes.ignore stem/prereq.py => 'asn1' imported but unused pyflakes.ignore stem/prereq.py => 'long_to_bytes' imported but unused -pyflakes.ignore stem/descriptor/__init__.py => redefinition of unused 'OrderedDict' from line 60 +pyflakes.ignore stem/descriptor/__init__.py => redefinition of unused 'OrderedDict' from line 61 pyflakes.ignore stem/util/str_tools.py => redefinition of function '_to_bytes_impl' from line 51 pyflakes.ignore stem/util/str_tools.py => redefinition of function '_to_unicode_impl' from line 57 pyflakes.ignore test/mocking.py => undefined name 'builtins' diff --git a/test/unit/response/protocolinfo.py b/test/unit/response/protocolinfo.py index 86309e7..e8bc884 100644 --- a/test/unit/response/protocolinfo.py +++ b/test/unit/response/protocolinfo.py @@ -156,7 +156,7 @@ class TestProtocolInfoResponse(unittest.TestCase): # - resolving the pid of the "tor" process # - using that to get tor's cwd - def call_mocking(command): + def call_mocking(command, default): if command == stem.util.system.GET_PID_BY_NAME_PGREP % "tor": return ["10"] elif command == stem.util.system.GET_CWD_PWDX % 10: diff --git a/test/unit/util/system.py b/test/unit/util/system.py index c59d2ee..a7b3d9b 100644 --- a/test/unit/util/system.py +++ b/test/unit/util/system.py @@ -97,12 +97,14 @@ def mock_call(base_cmd, responses): if base_cmd == command: return responses else: - return None + return default else: for cmd_completion in responses: if command == base_cmd % cmd_completion: return responses[cmd_completion] + return default + return functools.partial(_mock_call, base_cmd, responses) @@ -258,6 +260,8 @@ class TestSystem(unittest.TestCase): lsof_query = system.GET_PID_BY_FILE_LSOF % "/tmp/foo" mocking.mock(system.call, mock_call(lsof_query, ["4762"])) self.assertEquals(4762, system.get_pid_by_open_file("/tmp/foo")) + + mocking.mock(system.call, mocking.return_value([])) self.assertEquals(None, system.get_pid_by_open_file("/tmp/somewhere_else")) def test_get_cwd_pwdx(self): @@ -286,7 +290,7 @@ class TestSystem(unittest.TestCase): responses = { "75717": ["p75717", "n/Users/atagar/tor/src/or"], "1234": ["malformed output"], - "7878": None, + "7878": [], } mocking.mock(system.call, mock_call(system.GET_CWD_LSOF, responses)) @@ -306,7 +310,7 @@ class TestSystem(unittest.TestCase): "3333": ["JID", "bad data"], "4444": ["bad data"], "5555": [], - "6666": None + "6666": [] } mocking.mock(system.call, mock_call(system.GET_BSD_JAIL_ID_PS, responses)) diff --git a/test/util.py b/test/util.py index f87ecb3..4cdbdc2 100644 --- a/test/util.py +++ b/test/util.py @@ -243,7 +243,10 @@ def get_stylistic_issues(paths): issues = {} for path in paths: - pep8_output = stem.util.system.call("pep8 --ignore %s %s" % (ignored_issues, path)) + pep8_output = stem.util.system.call( + "pep8 --ignore %s %s" % (ignored_issues, path), + ignore_exit_status = True, + ) for line in pep8_output: line_match = re.match("^(.*):(\d+):(\d+): (.*)$", line) @@ -329,7 +332,10 @@ def get_pyflakes_issues(paths): issues = {} for path in paths: - pyflakes_output = stem.util.system.call("pyflakes %s" % path) + pyflakes_output = stem.util.system.call( + "pyflakes %s" % path, + ignore_exit_status = True, + ) for line in pyflakes_output: line_match = re.match("^(.*):(\d+): (.*)$", line)