PT_LOG and PT_STATUS event fields unspecifed

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

changed milestone to %Tor: 0.4.4.x-final

added 040-must 044-should component::core tor/tor documentation milestone::Tor: 0.4.4.x-final owner::atagar postfreeze-ok priority::high reviewer::ahf severity::normal sponsor::19-can status::needs-revision tor-pt tor-spec type::defect labels

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.

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

Trac:
Owner: N/A to dgoulet
Status: new to accepted
Keywords: N/A deleted, tor-spec, tor-pt added
Milestone: N/A to Tor: 0.4.0.x-final

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

Trac:
Keywords: N/A deleted, 040-must added

Taking this.

Trac:
Owner: dgoulet to ahf
Status: accepted to assigned

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.

Trac:
Status: assigned to needs_information

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.

Trac:
Status: needs_information to new

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

Trac:
Status: new to assigned

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

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

Trac:
Status: assigned to needs_review

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?

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?

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

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:

• The K/V line parser itself in very clean C code written by Nick: https://github.com/torproject/tor/blob/2f683465d4b666c5d8f84fb3b234ad539d8511cd/src/lib/encoding/kvline.c
• The current test cases for the K/V line parser: https://github.com/torproject/tor/blob/2f683465d4b666c5d8f84fb3b234ad539d8511cd/src/test/test_config.c#L5820

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.

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.

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

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?

Trac:
Owner: ahf to atagar
Keywords: 040-must deleted, N/A added
Status: needs_review to assigned

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

Trac:
Status: assigned to needs_revision

Trac:
Sponsor: N/A to Sponsor19-can

Trac:
Cc: N/A to catalyst

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

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to ahf

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?

Trac:
Status: needs_review to needs_revision

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

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?

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?

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.

Trac:
Keywords: N/A deleted, 040-must documentation added

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: 0.4.4.x-final

Trac:
Keywords: 040-must documentation deleted, 040-must documentation 044-should added

Mark tickets which are important or safe enough to look at post-freeze for 0.4.4.

Trac:
Keywords: 040-must documentation 044-should deleted, 044-should, postfreeze-ok, documentation, 040-must added

moved to tpo/core/tor#29136 (moved)