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

[Tor Bug Tracker & Wiki]

#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:

 >I refer you to the widely cited
 ​https://github.com/golang/go/wiki/CodeReviewComments#error-strings
 These are also a list of suggestions and there are many recommendations
 here that I do not like and would not recommend in this code base (for
 example the suggestion to use single character variable names).

 That being said I'm ok with generally not capitalizing error messages, and
 I agree with dcf's comment that error messages starting with identifiers
 should match the capitalization of the identifier.

 Some additional feedback:

 - Might as well be consistent. Looks like
 [https://github.com/shaneHowearth/snowflake/compare/linter-fixes#diff-
 bad3023f4e3a006241527544f4e9efd2R47 this] error message is still
 capitalized.

 - This
 [https://github.com/shaneHowearth/snowflake/commit/a2e9133a5f30672d3f680b763de009a9a9bf395b
 #diff-090a189afcd6b3645ff26daa525859f6L30 change] is still in the error
 commits and not in the general linter commit.

 - In
 [https://github.com/shaneHowearth/snowflake/commit/cf52da4619781ce9a660be895fe5c398cf6c119d
 #diff-79897051d7aac1f314600a930afebe9aR385 this] error message, it would
 be more correct to say something along the lines of:
 {{{
 log.Fatalf("reload of Geo IP databases on signal %s returned error: %v",
 signal, err)
 }}}
  (note also the change from `;` to `:` there)

 - In
 [https://github.com/shaneHowearth/snowflake/commit/e5a1bc32827482f971b003142acea3d9ea0b493a
 #diff-227e0da595ae35c9cef78b0e687e78f3R59 this] case the other connection
 the copyloop is copying to is not an ORPort, it's a SOCKS connection.

 - I am not a fan of
 [https://github.com/shaneHowearth/snowflake/commit/e5a1bc32827482f971b003142acea3d9ea0b493a
 #diff-0efd2b39fd0c8010b9dd51b4f07edf1bL157 this] change. I'd rather change
 `socksAcceptLoop` to not return an error since it's not necessary to
 handle it from the calling code. So something like:
 {{{
 func socksAcceptLoop(ln *pt.SocksListener, snowflakes
 sf.SnowflakeCollector) {
         defer ln.Close()
         log.Println("Started SOCKS listener.")
         for {
                 log.Println("SOCKS listening...")
                 conn, err := ln.AcceptSocks()
                 if err != nil {
                         if e, ok := err.(net.Error); ok && e.Temporary() {
                                 continue
                         }
                         log.Printf("SOCKS accept error: %s", err)
                 }
                 log.Println("SOCKS accepted: ", conn.Req)
                 err = sf.Handler(conn, snowflakes)
                 if err != nil {
                         log.Printf("handler error: %s", err)
                 }
         }
 }
 }}}

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