#28940: Add support for LOG to goptlib ---------------------------------------------+----------------------------- Reporter: dcf | Owner: dcf Type: enhancement | Status: | needs_revision Priority: Medium | Milestone: Component: Obfuscation/Pluggable transport | Version: Severity: Normal | Resolution: Keywords: goptlib | Actual Points: Parent ID: #28180 | Points: Reviewer: | Sponsor: ---------------------------------------------+----------------------------- Changes (by dcf):
* status: assigned => needs_revision Comment: Thanks for making this branch, ahf. First off: I'm really confused about the format of the LOG message. The discussion at #21879 and #28181 doesn't seem to have been updated after decisions got made on IRC. In addition to LOG, there's also STATUS? It would be nice if the format were in a spec, or at least in textual form in a ticket somewhere. Especially with regard to what byte values are allowed and which must be escaped, and how. Here's some quick review. * https://github.com/ahf/goptlib/commit/2d4cadf91f7da4fb43d8c1e85bb3c851fb660b0c (removing advice on how to spy on the stdout output stream) is inappropriate. The LOG feature does not replace this feature. While debugging, you might want to see for example CMETHOD line that won't just be logged verbatim by tor or whatever the controlling program is. And we won't be able to rely on the LOG feature for a long time yet. * I do not want goptlib to expose a `log.Logger` interface. That is a high-level application matter, not something for a low-level library like goptlib. If an application needs a `log.Logger`-compatible interface, it can write its own wrapper. I feel that there should be exactly one new exported function:\\ {{{ func Log(severity int, message string) }}} plus appropriate constants for severities. This is a tiny new feature; it doesn't need a new source file. * Don't use names like `kvline_escape_value`. Use camel-case names as https://golang.org/doc/effective_go.html#mixed-caps * I don't want a full-fledged key–value encoding library if we don't need it. I would rather have `Log` defined as something like\\ {{{ line("LOG", "SEVERITY=" + encodeValue(severity), "MESSAGE=" + encodeValue(message)) }}} In the `encodeValue` helper function, don't do a check like `kvline_value_needs_escape`. Just escape everything always. * No need to [https://github.com/ahf/goptlib/commit/c70bebb95b2a9585a31013bbee74fecc593802ea #diff-8ab1b4d8f6dfb248d21e27a7fcfee762R29 call bytes.Buffer.Grow]. It will grow automatically as you write to it. * In the test code, please also test: * Values containing `'\0'` (should panic?) * Values containing bytes >= `'\x80'` (e.g. UTF-8) (should panic?) * Values containing `'='` * Empty string Unfortunately, per https://spec.torproject.org/pt-spec Section 3.3, we are limited to US-ASCII minus `'\x00'` and `'\n'` in ''all'' PT output lines, so we need to test what happens when that is violated, even if the KV quoted string output allows other values. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28940#comment:2> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs