TorCtl Event Parsing Rewrite

added component::core tor/pytorctl parent::13630 priority::medium resolution::wontfix severity::normal status::closed type::enhancement labels

Change uploaded to the bug3679 branch of my repo: https://gitweb.torproject.org/atagar/pytorctl.git/shortlog/refs/heads/bug3679

Trac:
Status: new to needs_review

This is a good change that will make adding new events (e.g. STATUS_CLIENT) easier in the future. I am using this event parsing model in testing with my client and it works as well as the old model. (It feels faster to me, also, but I have no numbers to back that up.)

The only change I would make would be to chop up the commits differently, but that is more of a style issue. As an example, https://gitweb.torproject.org/atagar/pytorctl.git/commit/a6a23766d2a6be88e0742e5f92b20e1ecfdedf5d includes the addition of KW_ARG and _decodeFields() as well as changes to the Event and StatusEvent classes. I would split these out into 2 or 3 commits.

Trac:
Username: robinson

This looks good, and works with BwAuthority as expected.

2 of my BwAuthority scanners halted progress over the weekend. I am not sure how or if this is related, other than that these changes were merged for testing a day or so prior.

Replying to aagbsn:

2 of my BwAuthority scanners halted progress over the weekend. I am not sure how or if this is related, other than that these changes were merged for testing a day or so prior.

I restarted, and another scanner has frozen already. This looks like similar behavior to #3834 (moved), where the scanner freezes without warning. I have opened another ticket to track this issue here: #4015 (moved)

Trac:
Cc: N/A to aagbsn@extc.org

Ugh. Is there something in this paring code that could be throwing uncaught exceptions and/or quietly exiting? I see some new exceptions can be thrown from a few commits..

throwing uncaught exceptions and/or quietly exiting

If there is then it's a bug.

I see some new exceptions can be thrown from a few commits..

Please cite what you're concerned about. I'm juggling a few things so I don't have time to hunt through the code to guess what you mean. :)

I found a bug that might explain what's going on:

original code: (TorCtl.py:1367)

path_verb = path.strip().split(",")                                                             
path = []                                                                                       
for p in path_verb:                                                                             
  path.append(p.replace("~", "=").split("=")[0])

replacement code: (TorCtl.py:247)

if self.path:
  self.path = [p.replace("~", "=").split("=")[0] for p in self.path.split(",")]

See the missing 'strip()'? This might explain the BwAuthority issue reported in #4015 (moved) (descriptors do not match)

Trac:
3679-newdesc-event.patch

add missing strip()

For CIRC events the path is a positional argument. For instance... 650 CIRC 9 EXTENDED $59A3237FA7EB42CAD215FB6EA638ABF7F245873E~~zzz,$E5E3E9A472EAF7BE9682B86E92305DB4C71048EF~~Amunet2 PURPOSE=GENERAL

Which is parsed from the _decodeFields function:

1494     for entry in body.split():
1495       m = KW_ARG.match(entry)
1496 
1497       if m:
1498         k, v = m.groups()
1499         keywords[k] = v
1500       else:
1501         positional.append(entry)

Due to the split there shouldn't be any extra whitespace around the positional entries.

Are you sure that this is a bug? Are you able to reproduce any use case where the strip is needed?

Replying to atagar:

For CIRC events the path is a positional argument. For instance... 650 CIRC 9 EXTENDED $59A3237FA7EB42CAD215FB6EA638ABF7F245873E~~zzz,$E5E3E9A472EAF7BE9682B86E92305DB4C71048EF~~Amunet2 PURPOSE=GENERAL

Which is parsed from the _decodeFields function: {{{ 1494 for entry in body.split(): 1495 m = KW_ARG.match(entry) 1496 1497 if m: 1498 k, v = m.groups() 1499 keywords[k] = v 1500 else: 1501 positional.append(entry) }}}

Due to the split there shouldn't be any extra whitespace around the positional entries.

Are you sure that this is a bug? Are you able to reproduce any use case where the strip is needed?

You're right, I'm wrong. And my fix does not resolve BwAuthority issues. :-( Sorry.

I informed Mike via IRC, and I'm mentioning it here too: I plan to merge #2411 (moved) some time in December, barring misadventure -- so it's probably a good idea to get this stuff straightened out on about that timeframe.