[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5053 [Tor]: Fix IPv6 implementation for bridge statistics
#5053: Fix IPv6 implementation for bridge statistics
-----------------------------+----------------------------------------------
Reporter: karsten | Owner: ln5
Type: enhancement | Status: needs_review
Priority: major | Milestone: Tor: 0.2.3.x-final
Component: Tor | Version:
Keywords: ipv6 tor-bridge | Parent:
Points: | Actualpoints:
-----------------------------+----------------------------------------------
Changes (by ln5):
* status: needs_revision => needs_review
Comment:
Replying to [comment:33 nickm]:
> okay, I'll try to review the whole branch here...
>
> Stuff we could fix after merge:
Or before I guess. Please see branch bug5053-bug5055 in my public repo.
> * The duplicated code in the new geoip*ipv6() functions is scary. Gotta
refactor that out. Cut-and-pasting in programming is begging for bloat
and bugs.
Agreed. Commit 6a241ff3 merges geoip_ipv*_add_entry() and
geoip_ipv*_parse_entry().
> * Passing in6_addr by value? it is a 16-byte structure; that kind of
thing one usually passes by reference.
Gone with 6a241ff3.
> * Comparing in6_addrs with memcmp is iffy; there's no guarantee that
they can't contain padding or uninitialized fields. If we're going to use
memcmp, we should probably be comparing the s8_addr fields.
Fixed in commit e7e68b80.
Or do you prefer comparing the s6_addr32's without memcmp?
> * The comment that says that the in6_addrs are in "host order" seems
wrong.
Gone with e7e68b80.
> * The comment /* No alignment issue here, since _key really is a
pointer to uint32_t */ seems wrong.
Gone with e7e68b80.
> * The clear_geoip_db(); that got removed from geoip_load_file() : where
did it go? It looks like we just removed it entirely, which means that
reloading a geoip file will wind up with entries both from the old file
and the new one. That's a big bug.
geoip_load_file() frees geoip_ipv*_entries before adding new data.
geoip_ipv*_add_entry() only adds to geoip_countries data not already
present.
> * Gotta fix all the XXX5053; are there any left?
No.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5053#comment:34>
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