[tor-bugs] #9957 [Tor]: Tor should consider stderr output of transport proxies

[Tor Bug Tracker & Wiki]

#9957: Tor should consider stderr output of transport proxies
------------------------+--------------------------------
     Reporter:  wfn     |      Owner:
         Type:  defect  |     Status:  needs_revision
     Priority:  minor   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  tor-pt
Actual Points:          |  Parent ID:
       Points:          |
------------------------+--------------------------------

Comment (by wfn):

 Replying to [comment:16 asn]:
 > Replying to [comment:15 wfn]:
 > > Rebased once again (same branch) - to include an additional change
 which includes the `report_proxy_stderr` keyword that a user can look for
 when inspecting INFO-level tor log:
 > >
 > > https://github.com/wfn/tor/compare/bug_9957_2 ->
 > >
 > >   *
 https://github.com/wfn/tor/commit/9b3ef629225fa3a0fea0a7090a3ee8a4b14ffab1
 > >   *
 https://github.com/wfn/tor/commit/f8152a1fd36e5944ccf604a74bfbbb776e791786
 >
 > Nice. This looks cleaner!
 >
 > As a suggestion, to avoid needless nesting, maybe you would enjoy doing:
 > {{{
 > if (!proxy_err_output) {
 >     return
 > }
 > ... do everything ...
 > }}}
 > instead of:
 > {{{
 > if (proxy_err_output) {
 >       ... do everything ...
 > }
 > }}}
 > Up to your preference :)

 True, why have redundant nesting when you don't need to!

 > Also, AFAIK, we are using `STATIC` to mean "We should use static here,
 but we also need to unit test that function". Since you are not
 unittesting `report_proxy_stderr()` the function can be truly `static`
 like other functions in `src/or/transports.c` (for example
 `handle_finished_proxy()`).

 Aha, looked around and understood tor's use of `STATIC` just now (I
 think.) Yes, so this is one of those functions that can safely reside only
 in transports.c for sure and which does not need a test. Ok.

 (rebased again, fwiw - https://github.com/wfn/tor/compare/bug_9957_2)

 > Finally, we will need a changes file if we want this merged. I can write
 this for you, but you might want to write it on your own to get used to
 changes files for further Tor development :) As an example, see commit
 `71e0ca02b57f7945d922a8708a2c97815a9350ad`.

 For sure. :)

 Does the following make sense, or should it be reworded in some way (also,
 is this a 'minor feature' or just 'bugfix'? I'm not even sure.) (Kept
 character count per line at < 75.) Is this too verbose/detailed?

 {{{

   o Minor bugfixes (transport proxies):
     - Have Tor report stderr output of transport proxies to log.
       Previously we ignored standard error streams of transport proxies.
       This would sometimes cause confusion when they would fail
       prematurely (e.g. obfsproxy tracebacks get dumped to stderr.) We now
       make a notice when there's stderr output, and we log the output
       itself at INFO level. Implements ticket 9957.
 }}}

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9957#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online