Opened 2 years ago

Closed 7 months ago

#18918 closed defect (implemented)

Clarify directory and ORPort checking functions

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: easy, doc, code, refactor, technical-debt, tor-relay, review-group-31, review-group-32
Cc: ffmancera@… Actual Points:
Parent ID: Points: 1
Reviewer: teor, nickm Sponsor:

Description

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, 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

Child Tickets

Change History (21)

comment:1 Changed 23 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:2 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:3 Changed 16 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:4 Changed 15 months ago by nickm

Keywords: technical-debt tor-relay added

comment:5 Changed 10 months ago by aruna1234

Could you give more information as to what is the problem?

comment:6 Changed 10 months ago by teor

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?

comment:7 Changed 10 months ago by aruna1234

Actually I am a newbie, and I am searching for my first bug!
I am looking forward to get used to the workflow, and contribute effectively to the community.

comment:8 Changed 8 months ago by ffmancera

Cc: ffmancera@… added

comment:9 Changed 8 months ago by 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() and check_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 check options->BandwidthRate < MIN_BW_TO_ADVERTISE_DIRSERVER because we are checking the same on RelayBandwidthRate.
  • router_should_be_directory(): I don't think we need to check advertising != new_choice and then new_choice == 1 because we initialize both to 1 and within the function only new_choice could turn into 0. I already removed this if statement and it works correctly.
  • dir_server_mode() and decide_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_dirserv into router_should_be_directory. Let's decide about do or not these changes and I will work on them :-)

comment:10 in reply to:  9 Changed 8 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Points: 1

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() and check_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 check options->BandwidthRate < MIN_BW_TO_ADVERTISE_DIRSERVER because 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 check advertising != new_choice and then new_choice == 1 because we initialize both to 1 and within the function only new_choice could turn into 0. 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() and decide_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_dirserv into router_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!

comment:11 Changed 8 months ago by teor

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.

comment:12 Changed 8 months ago by ffmancera

Status: newneeds_review

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 :-)

comment:13 Changed 8 months ago by nickm

Keywords: review-group-31 added

comment:14 Changed 8 months ago by nickm

Status: needs_reviewneeds_revision

Only one thing I'm seeing here: the log message in router_should_be_directory_server() will sometimes refer to "DirDirectory Service support".

comment:15 in reply to:  14 Changed 8 months ago by ffmancera

Replying to nickm:

Only one thing I'm seeing here: the log message in router_should_be_directory_server() will sometimes refer to "DirDirectory Service support".

So should we introduce "Directory/DirDirectory" into log messages? Is there any other way to point out when it's each one?.

comment:16 Changed 8 months ago by nickm

Err, no: What I mean is that we should say either "Directory Service" or "DirPort" but not "DirDirectory Service".

comment:17 Changed 7 months ago by ffmancera

Sorry, I misunderstood you, it is fixed :-)

comment:18 Changed 7 months ago by ffmancera

Status: needs_revisionneeds_review

comment:19 Changed 7 months ago by nickm

Keywords: review-group-32 added

comment:20 Changed 7 months ago by teor

Reviewer: teor, nickm
Status: needs_reviewmerge_ready

Looks good, the log issue from the last review has been fixed

comment:21 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging to master!

Note: See TracTickets for help on using tickets.