[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #9729 [Tor]: Make bridges publish additional ORPort addresses in their descriptor
#9729: Make bridges publish additional ORPort addresses in their descriptor
-------------------------+-------------------------------------------------
Reporter: sqrt2 | Owner:
Type: | Status: needs_revision
enhancement | Milestone: Tor: 0.2.6.x-final
Priority: normal | Version: Tor: 0.2.5.1-alpha
Component: Tor | Keywords: ORPort, bridge, multiple,
Resolution: | addresses, 025-triaged
Actual Points: | Parent ID:
Points: |
-------------------------+-------------------------------------------------
Comment (by daube):
See inline updates below. I'll keep editing this comment as I finish
things off.
daube
Replying to [comment:29 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.
Fixed
> * It would probably be good idea to have a function to malloc and copy
a single address; that pattern happens a lot.
Implemented, it's called tor_addr_clone:
`tor_addr_t *tor_addr_clone(const tor_addr_t *src);`
> * 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().
Fixed
> * 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?
Refactored out. We now check if a router is contactable in a static
function. There's also a new static helper around
`node_af_reachable_since` that takes care of IPv4 & IPv6 called
`node_reachable_since`.
> * Local variables named "l" are begging for confusion with numberes
named "1".
Couldn't find this (sorry). Unless you're referring to variables named
`ri` or `rl`?
>
> 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?
Clean, tidied and a memory leak fixed (I think).
> * The other new functions here need documentation.
I've done my best
> * 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.
Fixed, they now have 'has/have' in their names. And params are `const X`.
> * all_listeners_have_replied looks a little expensive. We should make
sure it isn't in a critical path.
I'll let someone more familiar with the codebase as a whole speak to this.
> * Some of these functions should probably be static. Or STATIC, if
they are getting tested directly.
Namely? Does making `all_listeners_have_replied` and `addr_has_replied`
static make sence. Can't see any issues with this myself. I won't commit
this yet though.
>
> 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?
I've tried to mock out `get_interface_addresses_raw`, which seems to be
the root function in getting "real" addresses. Does this approach make
sense, and have I done so correctly?
>
> Throughout:
> * Probably most of the loops should be declaring the type as "const
tor_addr_t *", not "tor_addr_t *".
Will work on this.
> * 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.
It's clean now!
> * Most tor_addr_compare usage could be more cleanly written as
tor_addr_eq.
Done
> * 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.
Will work on this too.
> * 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.
This I will work on. I have gcov working now.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9729#comment:35>
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