Re: [tor-bugs] #31794 [Circumvention/Snowflake]: Errors swallowed

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#31794: Errors swallowed
-------------------------------------+--------------------------------
 Reporter:  sah                      |          Owner:  (none)
     Type:  defect                   |         Status:  needs_revision
 Priority:  Medium                   |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:  cohosh                   |        Sponsor:
-------------------------------------+--------------------------------
Changes (by cohosh):

 * status:  needs_review => needs_revision


Comment:

 Thanks for working on this! I have a few comments below for fixes. In
 general, it's easier for us to compare all the changes at once if you put
 your changes in a separate branch from master. That way (since this isn't
 a forked repo) we can compare your PR branch to the master branch and
 leave comments there.

 - A more informative error message
 [https://github.com/shaneHowearth/snowflake/blob/master/server-
 webrtc/snowflake.go#L33 here] would be useful. `a` is always the WebRTC
 connection and `b` is the ORPort. I would change this to be more like the
 error message in `server.go`
 [https://github.com/cohosh/snowflake/blob/master/server/server.go#L118
 here]

 - Similarly we can have a more informative error message
 [https://github.com/shaneHowearth/snowflake/blob/master/server-
 webrtc/snowflake.go#L256 here]. Maybe
  {{{log.Printf("error copying from os.Stdin to ioutil.Discard: %v",
 err)}}}

  Same with
 [https://github.com/shaneHowearth/snowflake/blob/master/server/server.go#L403
 here],
 [https://github.com/shaneHowearth/snowflake/commit/99a0fdfe88a10ec1bca04b99501080a2f4e43ad7
 #diff-227e0da595ae35c9cef78b0e687e78f3R64 here], and
 [https://github.com/shaneHowearth/snowflake/commit/2cf9f239820653b56b48b5f33b127ab8cfc39738
 #diff-ba4bd8a4477426567c409d66c2cf8a28R236 here]

 - As mentioned above,
 [https://github.com/shaneHowearth/snowflake/blob/master/broker/broker.go#L384
 this] geoip reload should log a Fatal error. See the call to the same
 function earlier in the code to see how to do it.

 - While I understand the desire to do so,
 [https://github.com/shaneHowearth/snowflake/commit/77e1ab458a6a742246cf4f42d7d54b2ea77c2814
 #diff-c8ef7ba143d251e41b143b2ab02f3733L136 this] shouldn't really be in
 this patch set since it has nothing to do with the ticket.
  Same with
 [https://github.com/shaneHowearth/snowflake/commit/77e1ab458a6a742246cf4f42d7d54b2ea77c2814
 #diff-345294112d730f1e04b863fb1e5c0981L184 this change]. There are more
 but I'm not going to list them all now.
  What I'm going to recommend is to squash all of the error handling
 changes into a single commit, and then have a separate commit for other
 linting changes.

 - Why did you make the change
 [https://github.com/shaneHowearth/snowflake/commit/9d6fabee2c6596fcf72518f31188219b66576525
 #diff-9d5c4d1c03bbea44d537ac915acf610aL65 here]? It changes the
 functionality of the code and should be reverted.

 Another general piece of feedback: your commit messages should be
 structured as follows:
 {{{
 Subject line, capitalized, no period, max 50 chars

 Commit message body, describe the changes exactly here. Use full
 sentences. Wrap the commit message body at 72 characters.
 }}}
 (your editor will probably do the wrapping for you)

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs