Refactor trace_inputs.py strace implementation to use -ff instead of -f.

Refactor trace_inputs.py strace implementation to use -ff instead of -f.

So each process has its own log file, reducing the race conditions inside
strace.

R=mad@chromium.org
BUG=98636
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137397

[M-A Ruel, 2012-05-11 15:57:04]

I'm building browser_tests to trace it to see if it helps with strace log
corruptions I've been seeing.

[M-A Ruel, 2012-05-15 02:28:41]

ping

[MAD, 2012-05-15 12:40:00]

Half way there... Sorry about that... Been interrupted by other things...

On Mon, May 14, 2012 at 10:28 PM, <maruel@chromium.org> wrote:

> ping
>
>
https://chromiumcodereview.**appspot.com/10356129/<https://chromiumcodereview...
>

[MAD, 2012-05-15 13:23:28]

Sorry about the delay, I had some trouble understanding the whole thing... Maybe
a comment at the top of the file describing the big picture would help...

Here are some comments, hopefully useful... Feel free to argue... :-)

BYE
MAD

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:255: UNFINISHED = ' <unfinished ...>'
why not include this string in the RE instead?

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:304: # Key is a the function name
???

Once properly phrased, it also need a '.' at the end... :-)

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:317: """Used by RelativePath. Also the initial
directory and not the
function doc string should not be longer than a single line. More details in
further paragraphs...

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:330: m = self.RE_KILLED.match(line)
Can't you use match instead of just 'm'?

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:359: if m.group(1) == '????':
Use a constant declared close to RE_HEADER for '????' ?

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:396:
self._handle_file(self.RE_EXECVE.match(args).group(1), result)
assert that you actually have a match?
Here and other places...

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:407: args = (self.RE_OPEN3.match(args) or
self.RE_OPEN2.match(args)).groups()
what if they are both None?

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:442: return self.initial_cwd
Why always the initial one only? Maybe add a comment to explain it.

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:447: def get_proc(self, pid):
this is misleading, it give the impression that we always get and that if it's
not there we should fail... I would rename to something like get_or_set_proc...
Or at least add comments here and where it's used...

[M-A Ruel, 2012-05-15 20:31:57]

ptal.

I updated the file's docstring, it was outdated anyway. I can make it clearer if
you want, anything you want me to clarify in particular?

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:255: UNFINISHED = ' <unfinished ...>'
On 2012/05/15 13:23:28, MAD wrote:
> why not include this string in the RE instead?

In this particular one, I thought an re object instance was overkill.

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:304: # Key is a the function name
On 2012/05/15 13:23:28, MAD wrote:
> ???
> 
> Once properly phrased, it also need a '.' at the end... :-)

cleaned up.

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:317: """Used by RelativePath. Also the initial
directory and not the
On 2012/05/15 13:23:28, MAD wrote:
> function doc string should not be longer than a single line. More details in
> further paragraphs...

cleaned up.

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:330: m = self.RE_KILLED.match(line)
On 2012/05/15 13:23:28, MAD wrote:
> Can't you use match instead of just 'm'?

I don't think it is more informative but done everywhere.

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:359: if m.group(1) == '????':
On 2012/05/15 13:23:28, MAD wrote:
> Use a constant declared close to RE_HEADER for '????' ?

done.

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:396:
self._handle_file(self.RE_EXECVE.match(args).group(1), result)
On 2012/05/15 13:23:29, MAD wrote:
> assert that you actually have a match?
> Here and other places...

The assert was simply for debugging purpose. Here it's roughly guaranteed to
match.

The API doesn't guarantee that the exception thrown is of type AssertionError.

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:407: args = (self.RE_OPEN3.match(args) or
self.RE_OPEN2.match(args)).groups()
On 2012/05/15 13:23:29, MAD wrote:
> what if they are both None?

Exception. E.g. it's assumed it'll work, otherwise the program will blow up
anyway.

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:442: return self.initial_cwd
On 2012/05/15 13:23:29, MAD wrote:
> Why always the initial one only? Maybe add a comment to explain it.

I added a docstring but the log processing Context instance has no concept of
current directory, it only knows the initial cwd of the initial process.

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:447: def get_proc(self, pid):
On 2012/05/15 13:23:29, MAD wrote:
> this is misleading, it give the impression that we always get and that if it's
> not there we should fail... I would rename to something like
get_or_set_proc...
> Or at least add comments here and where it's used...

done

[MAD, 2012-05-15 20:44:13]

Thanks... Almost there... One more question... And one small nit...

BYE
MAD

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:255: UNFINISHED = ' <unfinished ...>'
On 2012/05/15 20:31:57, Marc-Antoine Ruel wrote:
> On 2012/05/15 13:23:28, MAD wrote:
> > why not include this string in the RE instead?
> 
> In this particular one, I thought an re object instance was overkill.

What I meant was to have something like:
RE_UNFINISHED = re.compile(r'^([^\(]+).*$\s\<unfinished ...\>')

Then, you don't need to other constant and don't need to do an endswith first
and then remove it...

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:396:
self._handle_file(self.RE_EXECVE.match(args).group(1), result)
On 2012/05/15 20:31:57, Marc-Antoine Ruel wrote:
> On 2012/05/15 13:23:29, MAD wrote:
> > assert that you actually have a match?
> > Here and other places...
> 
> The assert was simply for debugging purpose. Here it's roughly guaranteed to
> match.
> 
> The API doesn't guarantee that the exception thrown is of type AssertionError.

OK, fair enough...

https://chromiumcodereview.appspot.com/10356129/diff/9002/tools/isolate/trace...
tools/isolate/trace_inputs.py:407: args = (self.RE_OPEN3.match(args) or
self.RE_OPEN2.match(args)).groups()
On 2012/05/15 20:31:57, Marc-Antoine Ruel wrote:
> On 2012/05/15 13:23:29, MAD wrote:
> > what if they are both None?
> 
> Exception. E.g. it's assumed it'll work, otherwise the program will blow up
> anyway.

OK...

https://chromiumcodereview.appspot.com/10356129/diff/14002/tools/isolate/trac...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/14002/tools/isolate/trac...
tools/isolate/trace_inputs.py:312: # The dict key is a the function name of the
pending call, like 'open'
is a the function name -> is the function name

[M-A Ruel, 2012-05-16 00:19:20]

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/2001/tools/isolate/trace...
tools/isolate/trace_inputs.py:255: UNFINISHED = ' <unfinished ...>'
On 2012/05/15 20:44:13, MAD wrote:
> What I meant was to have something like:
> RE_UNFINISHED = re.compile(r'^([^\(]+).*$\s\<unfinished ...\>')
> 
> Then, you don't need to other constant and don't need to do an endswith first
> and then remove it...

Done.

https://chromiumcodereview.appspot.com/10356129/diff/14002/tools/isolate/trac...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10356129/diff/14002/tools/isolate/trac...
tools/isolate/trace_inputs.py:312: # The dict key is a the function name of the
pending call, like 'open'
On 2012/05/15 20:44:15, MAD wrote:
> is a the function name -> is the function name

fixed.

[MAD, 2012-05-16 01:12:16]

LGTM...

Thanks!

[commit-bot: I haz the power, 2012-05-16 10:16:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10356129/7004

[commit-bot: I haz the power, 2012-05-16 11:38:24]

Change committed as 137397