Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  implemented
  technical-debt, tor-relay, review-group-31,|  Actual Points:
  review-group-32|
Parent ID:   | Points:  1
 Reviewer:  teor, nickm  |Sponsor:
-+-
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => implemented


Comment:

 Merging to master!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31,|
  review-group-32|
Parent ID:   | Points:  1
 Reviewer:  teor, nickm  |Sponsor:
-+-
Changes (by teor):

 * status:  needs_review => merge_ready
 * reviewer:   => teor, nickm


Comment:

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31 |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-
Changes (by ffmancera):

 * status:  needs_revision => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31 |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-

Comment (by ffmancera):

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31 |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-

Comment (by nickm):

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31 |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-

Comment (by ffmancera):

 Replying to [comment:14 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?.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor,   |  Actual Points:
  technical-debt, tor-relay, review-group-31 |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-
Changes (by nickm):

 * status:  needs_review => needs_revision


Comment:

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-
Changes (by ffmancera):

 * status:  new => needs_review


Comment:

 > 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
 [https://github.com/ffmancera/tor-1/tree/bug18918 branch bug18918]. All
 tests pass succesfully and Travis CI seems happy, I hope everything is
 fine :-)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-

Comment (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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:  1
 Reviewer:   |Sponsor:
-+-
Changes (by teor):

 * points:   => 1
 * milestone:  Tor: unspecified => Tor: 0.3.4.x-final


Comment:

 Replying to [comment:9 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!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-
Changes (by ffmancera):

 * cc: ffmancera@… (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (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?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by aruna1234):

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18918 [Core Tor/Tor]: Clarify directory and ORPort checking functions

[Tor Bug Tracker & Wiki]

#18918: Clarify directory and ORPort checking functions
-+-
 Reporter:  teor |  Owner:
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:  Tor:
 |  unspecified
Component:  Core Tor/Tor |Version:  Tor:
 |  0.2.8.1-alpha
 Severity:  Normal   | Resolution:
 Keywords:  easy, doc, code, refactor|  Actual Points:
  technical-debt tor-relay   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-
Changes (by nickm):

 * keywords:  easy, doc, code, refactor => easy, doc, code, refactor
 technical-debt tor-relay


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs