Opened 6 years ago

Closed 5 years ago

#3679 closed enhancement (wontfix)

TorCtl Event Parsing Rewrite

Reported by: atagar Owned by:
Priority: Medium Milestone:
Component: Torctl Version:
Severity: Keywords:
Cc: aagbsn@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The event parsing in TorCtl makes heavy use of regexes which is both unpleasant for readability and problematic when there's newly introduced attributes. The vast majority of events use a simple positional/keyword pattern so we can abstract much of this into something a little nicer.

This is also an issue for:
https://trac.torproject.org/projects/tor/ticket/2411

Child Tickets

Attachments (1)

3679-newdesc-event.patch (436 bytes) - added by aagbsn 6 years ago.
add missing strip()

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by atagar

Status: newneeds_review

comment:2 Changed 6 years ago by robinson

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.

comment:3 Changed 6 years ago by aagbsn

This looks good, and works with BwAuthority as expected.

comment:4 Changed 6 years ago by 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.

comment:5 in reply to:  4 Changed 6 years ago by aagbsn

Cc: aagbsn@… added

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, where the scanner freezes without warning. I have opened another ticket to track this issue here: #4015

comment:6 Changed 6 years ago by mikeperry

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..

comment:7 Changed 6 years ago by atagar

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. :)

comment:8 Changed 6 years ago by aagbsn

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 (descriptors do not match)

Changed 6 years ago by aagbsn

Attachment: 3679-newdesc-event.patch added

add missing strip()

comment:9 Changed 6 years ago by 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?

comment:10 in reply to:  9 Changed 6 years ago by aagbsn

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.

comment:11 Changed 6 years ago by nickm

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

comment:12 Changed 5 years ago by atagar

Resolution: wontfix
Status: needs_reviewclosed

Ahhh, the ticket that largely started stem. Ain't gonna happen so resolving.

https://blog.torproject.org/blog/torctl-deprecation-and-stem-plans

Note: See TracTickets for help on using tickets.