[tor-bugs] #5053 [Tor Bridge]: Fix IPv6 implementation for bridge statistics

[Tor Bug Tracker & Wiki]

#5053: Fix IPv6 implementation for bridge statistics
------------------------+---------------------------------------------------
 Reporter:  karsten     |          Owner:                    
     Type:  defect      |         Status:  needs_review      
 Priority:  major       |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Bridge  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by ln5):

 Replying to [comment:19 nils]:
 > > - I would've collapsed more of the functions into one too,
 > >   f.ex. geoip_get_country_by_ip() by adding a 'family' argument.
 >
 > This is a bit tricky seeing as the arguments and data structures are all
 different types.  geoip_get_country_by_addr already does the selection on
 address family and dispatches to the proper routine.  Or maybe I'm not
 understanding what you're suggesting here.

 Ah, I missed geoip_get_country_by_addr().  It indeed does what I
 suggest.  It'd be nice to hide geoip_get_country_by_ip() and
 geoip_get_country_by_ipv6() by making them static.  Unless callers
 will get inefficient or ugly by calling tor_addr_from_ipv4n()?


 > > - I'd renamed data types, lists and functions specific to IPv4 (like
 > >   struct geoip_entry_t) but I'd like to hear others view on this.
 >
 > I would like to hear others views on this as well.  I went for having a
 smaller changeset rather than renaming all of them, but it's easy enough
 to rename them.

 That change would go in a separate commit, preferrably predating your
 large commit with the real implementation.


 > > - There's tor_addr_compare() which I think should be used instead of
 memcmp().
 >
 > Unless I'm missing some, all of the memcmp cases are when we have
 specifically a in6_addr.  I guess we could use tor_addr_from_in6 to make a
 tor_addr_t and then use tor_addr_compare, but that seems like a lot of
 overhead and isn't nearly as clear.

 Aha!  struct geoip_ipv6_entry_t has in6_addr's, not tor_addr_t's.  Makes
 sense then.


 > I chose not to have a combined geoip database using tor_addr_t because
 of the additional memory usage (there are about 150,000 entries in the
 ipv4 geoip database) and it seemed a bit wasteful, but if we want to waste
 the extra 4 MB RAM we could use generic tor_addr_t addresses for the in-
 memory structure.

 No, separate structures is the right thing IMO.

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