Opened 6 months ago

Last modified 2 months ago

#29136 needs_revision defect

PT_LOG and PT_STATUS event fields unspecifed

Reported by: atagar Owned by: atagar
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, tor-pt, 040-must documentation
Cc: catalyst Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor: Sponsor19-can

Description

Recently Tor added PT_LOG and PT_STATUS events to the spec...

https://gitweb.torproject.org/torspec.git/commit/?id=3028cf1
https://gitweb.torproject.org/torspec.git/commit/?id=b38257e

Unfortunately the 'pt-spec.txt section 3.3.5' section they mention does not exist, and in looking around I can't find anything that describes what these event fields are defined as ('PT=' 'TYPE=', 'CONNECT=', etc).

I started to write a stem parser for these but can't continue until this is done (I can't parse events without knowing what fields they include).

David is aware of this and plans to has kindly offered to add the missing info...

22:24 <+atagar> dgoulet: Your control-spec addition to descript PT_LOG and PT_STATUS
                cite a pt-spec section 3.3.4 which does not exist.
22:24 <+atagar> s/descript/describe
22:29 <+atagar> dgoulet: Huh. I'm not spotting anything that lists the keyword
                arguments ('PT=' and 'SEVERITY=') so guess the sections simply
                missing from the spec. I need that for stem support so please
                give me a nudge when the event spec's done. :)
22:59 <+dgoulet> atagar: oh hmmm I'll fix that sorry
23:17 <+atagar> Thanks! Much appreciated. :)

Child Tickets

Change History (30)

comment:1 Changed 6 months ago by atagar

On a side note I'd appreciate it if the control-spec lists all the event fields rather than instructing the reader to see the pt-spec. It's fine to cite the pt-spec for information, but when it comes the basic event format the control-spec should include everything the reader needs.

I watch the control-spec and dir-spec to keep informed about control and directory specification changes. Reshuffling event formatting under other specs makes my life harder, and arguably makes our control-spec incomplete.

comment:2 in reply to:  1 Changed 6 months ago by teor

Replying to atagar:

On a side note I'd appreciate it if the control-spec lists all the event fields rather than instructing the reader to see the pt-spec. It's fine to cite the pt-spec for information, but when it comes the basic event format the control-spec should include everything the reader needs.

I watch the control-spec and dir-spec to keep informed about control and directory specification changes. Reshuffling event formatting under other specs makes my life harder, and arguably makes our control-spec incomplete.

If we duplicate information in control-spec and pt-spec, then they will get out of sync. Here's two options to avoid that:

  1. put the relevant information in control-spec, and point pt-spec to control-spec
  2. put the basic syntax in control-spec, and then say that additional fields may be added, and point to pt-spec for details

Whatever we decide, we should make sure that we are consistent with our other specs.
For example, the control-spec directory GETINFOs have basic info like URLs and descriptions, but the exact format is in dir-spec (like option 2).

comment:3 Changed 6 months ago by dgoulet

Keywords: tor-spec tor-pt added
Milestone: Tor: 0.4.0.x-final
Owner: set to dgoulet
Status: newaccepted

comment:4 Changed 5 months ago by nickm

Keywords: 040-must added

Marking tickets as 040-must based on triage with dgoulet.

comment:5 Changed 4 months ago by ahf

Owner: changed from dgoulet to ahf
Status: acceptedassigned

Taking this.

comment:6 Changed 4 months ago by ahf

Status: assignedneeds_information

I'm not a big fan of specifying the same structures both places, but I think we should extend control spec with information about the SEVERITY of the message in PT_LOG. Then I think that message type would be complete.

For PT_STATUS I think you should wait with doing anything fancy handling for now. We still don't have any PT's that emits anything useful here. If you want to implement some basic handling for this in stem, I suggest you emit an event which contains a dictionary of strings to string and write a parser for the serialization format we use (see tor's kv_line implementation).

atagar: Would an acceptable fix for this ticket be to specify SEVERITY for PT_LOG? As far as I can tell that is the only piece missing in control-spec.txt to be able to implement a PT_LOG handler purely from reading the control-spec.txt itself without ever having a look at the PT specification.

comment:7 Changed 4 months ago by atagar

Status: needs_informationnew

Hi Alex. I'd appreciate if the control-spec includes all information necessary for parsers. Just about every other event type specifies in detail what each field within the event will contain...

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n3240
https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n2805
... etc...

PT_LOG and PT_STATUS break this convention by merely describing their fields. They do not specify what parsers should expect...

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n3284

Personally I don't have an interest in what these events do or even if they're implemented. My sole concern is our spec, as that is what Stem will implement. If PT_STATUS is moot then lets remove it from the spec.

comment:8 Changed 4 months ago by ahf

Status: newassigned

Great. I will try to make an update for control-spec.txt.

comment:9 Changed 4 months ago by atagar

Wonderful, thanks Alex! If you'd like any help then just let me know. :)

comment:10 Changed 4 months ago by ahf

Status: assignedneeds_review

I have added a commit to my branch 'ahf/29136' here. Please have a look at: https://github.com/ahf/torspec/tree/ahf/29136

comment:11 Changed 4 months ago by atagar

Thanks Alex. This doesn't match the specification conventions used by the rest of the spec file to be fair they're pretty arcane. ;P

How about I take a stab at this and you can tell me if the formatting constraints are acceptable?

comment:12 Changed 4 months ago by ahf

If you are up for it, that would be cool. I think the big problem I saw was the lack of definition for the severity message.

Perhaps you are more interested in some formal grammar or something for the key/value parser?

comment:13 Changed 4 months ago by atagar

Yup. The formal parser grammar and future proofing details are the tidbits I'm interested in. I'll aim to whip up something for us to discuss in a few days (juggling this with both work and a sore throat).

comment:14 Changed 4 months ago by ahf

That would be pretty nice to have both in the PT spec, but also in the Control spec, as it's the same format that is going through here and we plan on use this in more places in the future as it makes it easier to extend the messages later on.

Let me link you with some relevant code:

comment:15 Changed 4 months ago by atagar

Hi ahf, sorry about the delay. Just finished getting over my cold and now looking at a daunting load of work from my dayjob. I probably won't be able to get to this for a few weeks.

comment:16 Changed 4 months ago by ahf

Cool! I'm going to talk with the other network-team members on Monday about whether we should move this one out of the 040-must keyword.

Thanks for the update.

comment:17 Changed 4 months ago by atagar

Oh yeah. For what it's worth there's no rush on my end about this. It's just specification detail. :P

comment:18 Changed 4 months ago by teor

We can move this ticket from 040-must, and make sure we do it in 0.4.1 instead?
Should we tag it with a sponsor?

comment:19 Changed 4 months ago by teor

Keywords: 040-must removed
Owner: changed from ahf to atagar
Status: needs_reviewassigned

comment:20 Changed 4 months ago by teor

Status: assignedneeds_revision

I think it's ok to update the spec after the 0.4.0 release.

comment:21 Changed 4 months ago by teor

Sponsor: Sponsor19-can

comment:22 Changed 3 months ago by catalyst

Cc: catalyst added

comment:23 Changed 3 months ago by atagar

Status: needs_revisionneeds_review

Hi ahf, apologies for the delay. I've pushed a prospective patch to the 'bug29136' branch of my repository. Please read the commit message for a few open questions.

https://gitweb.torproject.org/user/atagar/torspec.git/commit/?h=bug29136&id=0676715
https://gitweb.torproject.org/user/atagar/torspec.git/log/?h=bug29136

comment:24 Changed 3 months ago by asn

Reviewer: ahf

comment:25 Changed 3 months ago by ahf

Status: needs_reviewneeds_revision

Thanks for working on this, atagar!

Going to try to answer your three questions here:

  • Do PT_STATUS events exist or not? The prior spec confusingly said they're both a thing and unimplemented. We should either implement PT_STATUS or remove it from this specification (and tor's codebase, if present).

They exist, but as a reserved keyword right now. We still don't use them yet, so the spec will need revision as we add items here.

  • Are other severities permissible? And are we sure we want the 'severity' to be lowercase? Other events use upercase enumerations. Changing this would probably require a change in tor.

I think lowercase is fine here.

  • I've defined all PT_LOG and PT_STATUS attributes to be optional. Should any of them be mandatory? If so then please drop their square brackets.

For PT_LOG all 3 of the keys are mandatory: SEVERITY, PT, and MESSAGE. The MESSAGE value could be empty, but most likely wont be.

For PT_STATUS the PT and TRANSPORT is mandatory. Everything else is optional.

Does this help for clarification?

comment:26 Changed 3 months ago by atagar

Hi Alex. Is there a ticket for implementing PT_STATUS? If so then this spec patch should either await it (if PT_STATUS's merge is imminent), or we should remove PT_STATUS until it's ready.

Maybe I'm missing something but it sounds like a mistake to spec or reserve the keyword for something that doesn't exist. Doing so risks that five years from now someone will ask "What is this PT_STATUS thing?" and the answer will be "something we started to spec out but never got around to".

Better to merge and spec PT_STATUS after it's done. :)

comment:27 Changed 3 months ago by ahf

PT_STATUS *is* merged into tor.git, but no message structures are currently *reserved* for it there. It is in there such that PT developers can experiment with what messages that makes sense to have. Since it has the key/value format, they can practically describe any structure they find useful and we can then reserve them in the spec. Does that make sense?

comment:28 Changed 3 months ago by atagar

Hi Alex. Sorry, but by 'exists' I mean "I as a tor user can call 'SETEVENTS PT_STATUS' in some scenario get something back. Does that exist today?

comment:29 in reply to:  26 Changed 3 months ago by teor

Replying to atagar:

Hi Alex. Is there a ticket for implementing PT_STATUS? If so then this spec patch should either await it (if PT_STATUS's merge is imminent), or we should remove PT_STATUS until it's ready.

Maybe I'm missing something but it sounds like a mistake to spec or reserve the keyword for something that doesn't exist. Doing so risks that five years from now someone will ask "What is this PT_STATUS thing?" and the answer will be "something we started to spec out but never got around to".

Better to merge and spec PT_STATUS after it's done. :)

We have reserved cell types and protocol versions before, and then never implemented them. atagar is right: if it is proposed, it belongs in a proposal. If it's in tor, it belongs in the spec.

comment:30 Changed 2 months ago by nickm

Keywords: 040-must documentation added
Note: See TracTickets for help on using tickets.