Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in
T
Tor
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 1,067
    • Issues 1,067
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
  • Merge Requests 20
    • Merge Requests 20
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
  • Operations
    • Operations
    • Incidents
    • Environments
  • Analytics
    • Analytics
    • CI / CD
    • Repository
    • Value Stream
  • Members
    • Members
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar

GitLab is used only for code review, issue tracking and project management. Canonical locations for source code are still https://gitweb.torproject.org/ https://git.torproject.org/ and git-rw.torproject.org.

  • The Tor Project
  • Core
  • Tor
  • Issues
  • #22203

Closed
Open
Opened May 09, 2017 by Roger Dingledine@armaReporter

Should a hup reload the geoip files?

In config.c in config_maybe_load_geoip_files_() we have this line:

  /* XXXX Reload GeoIPFile on SIGHUP. -NM */

and sure enough, it looks like there's nothing in main.c's do_hup() that would make us reload the geoip files.

It would be relatively easy to do I think:

  • In do_hup(), right around the call to routerlist_reset_warnings(), we call something in geoip.c that tells it to no longer consider itself initialized. Maybe that call is something like clear_geoip_db().
  • Then in config_maybe_load_geoip_files_(), since geoip_is_loaded() returns 0, it loads them.

Things get tricky though: catalyst asked if reloading the geoip files messes up the statistics gathering.

If reloading the geoip files does mess up statistics gathering, we have ourselves a minor bug, since config_maybe_load_geoip_files_() does reload them if the GeoIPFile location changes.

But it turns out to be more complicated than that, since geoip_load_file() only clears selective things: it clears geoip_ipv4_entries and geoip_ipv6_entries, but it leaves geoip_countries alone! And I see elsewhere, at the bottom of geoip_note_client_seen(), that we are keeping statistics directly in the geoip_countries smartlist:

      geoip_country_t *country = smartlist_get(geoip_countries, country_idx);
      ++country->n_v3_ns_requests;

So we would not want to call clear_geoip_db() on hup, or we'd lose some stats.

I guess that means if we want to make do_hup reload the geoip stats file, the better (non-invasive) plan is to have a boolean want_to_reload_geoip{4,6} in geoip.c that we turn on in do_hup, and then we check the right boolean in geoip_is_loaded().

I think there's a big argument for growing some good unit tests here, around "what happens when we reload this, collect those stats, reload that, examine those other stats, etc", since things are either subtly broken right now, or awfully fragile.

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Assignee
Assign to
None
Milestone
None
Assign milestone
Time tracking
None
Due date
None
Reference: tpo/core/tor#22203