Clarify directory and ORPort checking functions
The OR and dir checking functions in router.c are somewhat confusing. We could document them better, or modify their names, or restructure.
In #18616 (moved), we added:
- decide_to_advertise_begindir
We already had:
- check_whether_orport_reachable
- check_whether_dirport_reachable
- router_has_bandwidth_to_be_dirserver
- router_should_be_directory_server
- dir_server_mode
- decide_to_advertise_dirport
- consider_testing_reachability
Activity
changed milestone to %Tor: 0.3.4.x-final
added code component::core tor/tor doc easy milestone::Tor: 0.3.4.x-final points::1 priority::medium refactor resolution::implemented review-group-31 review-group-32 reviewer::nickm reviewer::teor severity::normal status::closed technical-debt tor-relay type::defect version::tor 0.2.8.1-alpha labels
The names of the functions are confusing. We want them to have a good structure and a consistent naming scheme. We should improve the comments, rename, combine, or split some of them.
So maybe this is not an easy ticket after all. Do you like reading code, understanding it, and giving it a better structure?
Trac:
Cc: N/A to ffmancera@riseup.netI have been reviewing the code these days. I don't think we should rename any function but I want to propose some changes in the code structure. I will list all the reviewed functions with my notes about them.
-
check_whether_orport_reachable()andcheck_whether_dirport_reachable(): I think they are fine, nice documentation and names. -
router_has_bandwidth_to_dirserver(): Poor documentation, I already improved it a bit. Also I think we don't need to checkoptions->BandwidthRate < MIN_BW_TO_ADVERTISE_DIRSERVERbecause we are checking the same on RelayBandwidthRate. -
router_should_be_directory(): I don't think we need to checkadvertising != new_choiceand thennew_choice == 1because we initialize both to1and within the function onlynew_choicecould turn into0. I already removed this if statement and it works correctly. -
dir_server_mode()anddecide_to_advertise_dirport(): I think they are fine, nice documentation and names.
About combine functions, I think we could get
router_has_bandwidth_to_be_dirservintorouter_should_be_directory. Let's decide about do or not these changes and I will work on them :-)-
Replying to ffmancera:
I have been reviewing the code these days. I don't think we should rename any function but I want to propose some changes in the code structure. I will list all the reviewed functions with my notes about them.
-
check_whether_orport_reachable()andcheck_whether_dirport_reachable(): I think they are fine, nice documentation and names. -
router_has_bandwidth_to_dirserver(): Poor documentation, I already improved it a bit. Also I think we don't need to checkoptions->BandwidthRate < MIN_BW_TO_ADVERTISE_DIRSERVERbecause we are checking the same on RelayBandwidthRate.
Let's rename this to router_has_bandwidth_to_be_dirserver(). "to dirserver" is confusing, it could refer to another dirserver.
See my note below about removing conditions that depend on external code.
-
router_should_be_directory(): I don't think we need to checkadvertising != new_choiceand thennew_choice == 1because we initialize both to1and within the function onlynew_choicecould turn into0. I already removed this if statement and it works correctly.
When we remove statements because they are obviously correct, we usually insert a tor_assert_nonfatal_once() or if(BUG()) to make sure the condition holds. If the condition depends on code outside the function, please check it inside the function.
For consistency, let's rename this to router_should_be_dirserver().
-
dir_server_mode()anddecide_to_advertise_dirport(): I think they are fine, nice documentation and names.
Let's rename decide_to_advertise_dirport() to router_should_advertise_dirport() for consistency.
About combine functions, I think we could get
router_has_bandwidth_to_be_dirservintorouter_should_be_directory.If both functions are short, this is ok. But if the combined function is longer than a standard terminal window (24 lines), let's not combine them.
Let's decide about do or not these changes and I will work on them :-)
They sound fine to me. Thanks for working on this ticket!
Trac:
Points: N/A to 1
Milestone: Tor: unspecified to Tor: 0.3.4.x-final-
Oh, you left out a few:
decide_to_advertise_begindir() -> router_should_advertise_begindir()
consider_testing_reachability():
Maybe we could split this into a boolean function that doesn't change state: router_should_check_reachability(). And a function that calls that function and changes state: router_do_reachability_checks().
You might also want to check the code and documentation in these functions.
Let's rename this to router_has_bandwidth_to_be_dirserver(). "to dirserver" is confusing, it could refer to another dirserver.
Sorry I wrote it wrong in the ticket comment.
When we remove statements because they are obviously correct, we usually insert a tor_assert_nonfatal_once() or if(BUG()) to make sure the condition holds. If the condition depends on code outside the function, please check it inside the function.
tor_assert_nonfatal_once()gave me a warning on tests so the conditions were needed.If both functions are short, this is ok. But if the combined function is longer than a standard terminal window (24 lines), let's not combine them.
We shouldn't combine them because the combined function is longer than 24 lines.
Everything else was done, please check my github branch bug18918. All tests pass succesfully and Travis CI seems happy, I hope everything is fine :-)
Trac:
Status: new to needs_reviewmoved to tpo/core/tor#18918 (closed)
