Bazaar

Merge lp:~mbp/bzr/626679-strace into lp:bzr

  1. 626679-strace
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 5402
Proposed branch: lp:~mbp/bzr/626679-strace
Merge into: lp:bzr
Diff against target: 184 lines (+54/-28)
3 files modified
NEWS (+4/-0)
bzrlib/strace.py (+22/-3)
bzrlib/tests/test_strace.py (+28/-25)
To merge this branch: bzr merge lp:~mbp/bzr/626679-strace
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Review via email: mp+34157@code.launchpad.net

Commit message

disable strace tests broken under maverick

Description of the change

Ubuntu recently changed to stop users tracing their own processes by default as a security measure, which means that 'bzr selftest -s bt.test_strace' fails on maverick.

Unfortunately strace doesn't give you a clear error code to tell you that you hit this.

We don't actually use strace in any other tests at the moment, but it has been useful in the past for ad-hoc performance investigation.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

+1

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Revision history for this message
Andrew Bennetts (spiv) wrote :

PQM seems to have hung while processing this. The circumstantial evidence from spm is that it was probably executing the strace tests at the time: there was an strace subprocess, and killing it caused another strace subprocess to be spawned. Hopefully some informative tracebacks/errors are on their way back to Martin from PQM.

Revision history for this message
Martin Pool (mbp) wrote :

Thanks.

That actually pushes me a bit more towards deleting/disabling the
tests, but leaving the facility. It's useful to have it, but it seems
silly to have test hangs for something that's not actively used other
than in the tests.

--
Martin

Revision history for this message
Andrew Bennetts (spiv) wrote :

Martin Pool wrote:
> Thanks.
>
> That actually pushes me a bit more towards deleting/disabling the
> tests, but leaving the facility. It's useful to have it, but it seems
> silly to have test hangs for something that's not actively used other
> than in the tests.

I think that's a reasonable compromise. Maybe leave the tests there but
skipped? It doesn't seem like something worth spending large amounts of
time on. The next person to work on the strace helpers can look at
fixing their tests if they like.

Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-09-01 08:02:42 +0000
3+++ NEWS 2010-09-01 08:49:39 +0000
4@@ -142,6 +142,10 @@
5 fetching from repositories with affected revisions.
6 (Andrew Bennetts, #522637)
7
8+* strace test-helper tests cope with the new Ubuntu policy of not allowing
9+ users to attach to their own processes by default.
10+ (Martin Pool, #626679)
11+
12 * ``Transport.stat`` on a symlink, including a transport pointing directly
13 to a symlink, now returns information about the symlink.
14 (Martin Pool)
15
16=== modified file 'bzrlib/strace.py'
17--- bzrlib/strace.py 2009-03-23 14:59:43 +0000
18+++ bzrlib/strace.py 2010-09-01 08:49:39 +0000
19@@ -23,6 +23,9 @@
20 import subprocess
21 import tempfile
22
23+from bzrlib import errors
24+
25+
26 # this is currently test-focused, so importing bzrlib.tests is ok. We might
27 # want to move feature to its own module though.
28 from bzrlib.tests import Feature
29@@ -46,14 +49,17 @@
30 # capture strace output to a file
31 log_file = tempfile.NamedTemporaryFile()
32 log_file_fd = log_file.fileno()
33+ err_file = tempfile.NamedTemporaryFile()
34 pid = os.getpid()
35 # start strace
36 strace_cmd = ['strace', '-r', '-tt', '-p', str(pid), '-o', log_file.name]
37 if follow_children:
38 strace_args.append('-f')
39+ # need to catch both stdout and stderr to work around
40+ # bug 627208
41 proc = subprocess.Popen(strace_cmd,
42 stdout=subprocess.PIPE,
43- stderr=subprocess.STDOUT)
44+ stderr=err_file.fileno())
45 # Wait for strace to attach
46 attached_notice = proc.stdout.readline()
47 # Run the function to strace
48@@ -65,18 +71,31 @@
49 log_file.seek(0)
50 log = log_file.read()
51 log_file.close()
52- return result, StraceResult(log)
53+ # and stderr
54+ err_file.seek(0)
55+ err_messages = err_file.read()
56+ err_file.close()
57+ # and read any errors
58+ if err_messages.startswith("attach: ptrace(PTRACE_ATTACH,"):
59+ raise StraceError(err_messages=err_messages)
60+ return result, StraceResult(log, err_messages)
61+
62+
63+class StraceError(errors.BzrError):
64+
65+ _fmt = "strace failed: %(err_messages)s"
66
67
68 class StraceResult(object):
69 """The result of stracing a function."""
70
71- def __init__(self, raw_log):
72+ def __init__(self, raw_log, err_messages):
73 """Create a StraceResult.
74
75 :param raw_log: The output that strace created.
76 """
77 self.raw_log = raw_log
78+ self.err_messages = err_messages
79
80
81 class _StraceFeature(Feature):
82
83=== modified file 'bzrlib/tests/test_strace.py'
84--- bzrlib/tests/test_strace.py 2009-03-23 14:59:43 +0000
85+++ bzrlib/tests/test_strace.py 2010-09-01 08:49:39 +0000
86@@ -1,5 +1,4 @@
87-# Copyright (C) 2007 Canonical Ltd
88-# Authors: Robert Collins <robert.collins@canonical.com>
89+# Copyright (C) 2007, 2010 Canonical Ltd
90 #
91 # This program is free software; you can redistribute it and/or modify
92 # it under the terms of the GNU General Public License as published by
93@@ -22,34 +21,26 @@
94 import threading
95
96 from bzrlib import (
97+ strace,
98 tests,
99 )
100 from bzrlib.strace import StraceFeature, strace_detailed, StraceResult
101
102
103-class TestStraceFeature(tests.TestCaseWithTransport):
104-
105- def test_strace_detection(self):
106- """Strace is available if its runnable."""
107- try:
108- proc = subprocess.Popen(['strace'],
109- stderr=subprocess.PIPE,
110- stdout=subprocess.PIPE)
111- proc.communicate()
112- found_strace = True
113- except OSError, e:
114- if e.errno == errno.ENOENT:
115- # strace is not installed
116- found_strace = False
117- else:
118- raise
119- self.assertEqual(found_strace, StraceFeature.available())
120-
121-
122 class TestStrace(tests.TestCaseWithTransport):
123
124 _test_needs_features = [StraceFeature]
125
126+ def setUp(self):
127+ # NB: see http://pad.lv/626679 and
128+ # <https://code.launchpad.net/~mbp/bzr/626679-strace/+merge/34157>:
129+ # testing strace by connecting to ourselves has repeatedly caused
130+ # hangs in running the test suite; these are fixable given enough
131+ # determination but given that strace is not used by any other tests
132+ # at the moment and that it's only test-support code, we just leave it
133+ # untested -- mbp 20100901
134+ raise tests.TestSkipped("strace selftests are broken and disabled")
135+
136 def _check_threads(self):
137 # For bug #226769, it was decided that the strace tests should not be
138 # run when more than one thread is active. A lot of tests are currently
139@@ -61,14 +52,26 @@
140 raise tests.KnownFailure(
141 '%d active threads, bug #103133 needs to be fixed.' % active)
142
143+ def strace_detailed_or_skip(self, *args, **kwargs):
144+ """Run strace, but cope if it's not allowed"""
145+ try:
146+ return strace_detailed(*args, **kwargs)
147+ except strace.StraceError, e:
148+ if e.err_messages.startswith(
149+ "attach: ptrace(PTRACE_ATTACH, ...): Operation not permitted"):
150+ raise tests.TestSkipped("ptrace not permitted")
151+ else:
152+ raise
153+
154 def test_strace_callable_is_called(self):
155 self._check_threads()
156
157 output = []
158 def function(positional, *args, **kwargs):
159 output.append((positional, args, kwargs))
160- strace_detailed(function, ["a", "b"], {"c": "c"},
161- follow_children=False)
162+ self.strace_detailed_or_skip(
163+ function, ["a", "b"], {"c": "c"},
164+ follow_children=False)
165 self.assertEqual([("a", ("b",), {"c":"c"})], output)
166
167 def test_strace_callable_result(self):
168@@ -76,7 +79,7 @@
169
170 def function():
171 return "foo"
172- result, strace_result = strace_detailed(function,[], {},
173+ result, strace_result = self.strace_detailed_or_skip(function,[], {},
174 follow_children=False)
175 self.assertEqual("foo", result)
176 self.assertIsInstance(strace_result, StraceResult)
177@@ -87,6 +90,6 @@
178
179 def function():
180 self.build_tree(['myfile'])
181- unused, result = strace_detailed(function, [], {},
182+ unused, result = self.strace_detailed_or_skip(function, [], {},
183 follow_children=False)
184 self.assertContainsRe(result.raw_log, 'myfile')