Hi Damian!
Thanks a lot for your feedback! The proposed event formats are not at all set in stone. If we can change formats to make events easier to parse, by all means let's do that. You probably have more experience with the control port than anyone at Tor, so I say let's listen to you.
On 2/7/13 5:33 PM, Damian Johnson wrote:
Hi Karsten. Looks good to me, just a few comments on the spec parts...
5.1. Adding an ID field to ORCONN events
Please define the format of ConnId. This would belong in section 2.4, expanding...
; Unique identifiers for streams or circuits. Currently, Tor only ; uses digits, but this may change StreamID = 1*16 IDChar CircuitID = 1*16 IDChar IDChar = ALPHA / DIGIT
Good point, will do.
5.2. Bandwidth used on an OR or DIR or EXIT connection
Minor nit pick, but the normal way of showing an enumeration is...
ConnType = "OR" / "DIR" / "EXIT"
Please make it clear in the spec if new ConnType are allowed or not.
Yes, new types should be allowed, though I think it will be just these three types for now.
I wonder if we should avoid restricting connection types in the spec by defining this argument as connection type _string_ as opposed to an enumeration. That would be similar to cell type strings in CELL_STATS events. Has the advantage that we don't have to touch the spec whenever we add another connection type. Does that make sense?
5.3. Per-circuit cell stats
Again, please specify the format of all of these cells. Most are obviously numeric, but a formal specification helps answer questions like 'can I assume this can't be negative?'.
All numbers are non-negative integers. Will specify this more formally.
Are you sure that you want to use positional arguments for all of these? If you do then you'll lose a lot of flexibility. Making them positional means...
- these values will always be mandatory, even if you want to deprecate
them in the future
- you'll always need to keep this ordering, with new values appended to the end
A fine question. Here are two examples of this event:
CELL_STATS 8 47110 15 created=1;relay=1 created=1;relay=1 created=0;relay=0 39943 16 create=1 create=1 create=0
CELL_STATS 8 47110 15 - - - 39943 16 relay_early=1 relay_early=1 relay_early=0
Can you suggest a better format, maybe one without positional arguments?
"650" SP "TB_EMPTY" SP ["GLOBAL" || "RELAY" || "ORCONN" SP ConnID] SP ReadBucketEmpty SP WriteBucketEmpty SP LastRefill CRLF
Ewww! No, please don't do this. Pretty please with a cherry on top?
Ewww, cherries... ;)
Having either one *or* two positional arguments for the first slot makes this a special snowflake among tor events (and hence need special handling). ConnID should be an optional keyword argument instead, documented to only exist if the event type is "ORCONN".
Sure, sounds doable. Will fix that.
Same cautioning as before about positional vs keyword arguments, and defining the values.
I'm fine changing positional arguments to keyword arguments. Maybe we should do that for all three new events. Want to suggest new event formats with keyword arguments to make them easier to parse by Stem and friends? I'll wait with making changes to the proposal until I hear back from you.
Thanks again for your very valuable feedback!
Cheers! Karsten