[tor-bugs] #9729 [Tor]: Make bridges publish additional ORPort addresses in their descriptor

[Tor Bug Tracker & Wiki]

#9729: Make bridges publish additional ORPort addresses in their descriptor
----------------------------+----------------------------------------------
     Reporter:  sqrt2       |      Owner:
         Type:              |     Status:  needs_review
  enhancement               |  Milestone:  Tor: 0.2.5.x-final
     Priority:  normal      |    Version:  Tor: 0.2.5.1-alpha
    Component:  Tor         |   Keywords:  ORPort bridge multiple addresses
   Resolution:              |  Parent ID:
Actual Points:              |
       Points:              |
----------------------------+----------------------------------------------

Comment (by nickm):

 Here's a preliminary code review.

 In address.c:
   * The code in get_stable_interface_address6 seems to assume that
 'family' is AF_INET or AF_INET6.  Maybe it should check that early on.
   * It would probably be good idea to have a function to malloc and copy a
 single address; that pattern happens a lot.
   * get_stable_interface_address6 looks like it could benefit from the
 "goto end" pattern of having a single point of cleanup.

 In config.c:
   * We use tor_assert, not assert().
   * XXX come back and re-review resolve_my_address.  It's too complicated.
 :(

 In dirserv.c:
   * Wow, that conditional in dirserv_set_router_is_running is pretty
 complicated.  Can we turn it into a function?
   * Local variables named "l" are begging for confusion with numberes
 named "1".

 In nodelist.c:
   * In node_set_last_reachability, what is the point of setting
 ar->addrport.port and ar->addrport.addr immediately after checking whether
 those fields are equal to the values you're about to set them to?  And why
 are we copying ap->addr into *addr?
   * The other new functions here need documentation.
   * addr_replied should have a name that makes clear that it's a
 predicate.  Perhaps  "addr_has_replied"? Similarly with
 all_listenres_have_Replied.  Also, its arguments should be const.
   * all_listeners_have_replied looks a little expensive.  We should make
 sure it isn't in a critical path.
   * Some of these functions should probably be static.  Or STATIC, if they
 are getting tested directly.

 test_addr.c:
   * Maybe some of these tests should use the test mocking functionality,
 so that they don't depend so heavily on the actual IP addresses of the
 running host?

 Throughout:
   * Probably most of the loops should be declaring the type as "const
 tor_addr_t *", not "tor_addr_t *".
   * Try building this branch after running configure with "--enable-gcc-
 warnings".  That will turn on a bunch of warnings that aren't on by
 default, but which we use to write cleaner code.
   * Most tor_addr_compare usage could be more cleanly written as
 tor_addr_eq.
   * Try to write if-else statements with braces on both the "if" and
 "else" clauses.  Leaving braces out makes the code a little harder to
 review, since I need to make sure that the indentation is right.
   * This could still use more testing.  Taking a look at the results of
 running the test coverage tools (see doc/HACKING for more info), I see
 that only about 1/3 of the new or modified lines have any tests reaching
 them.

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