[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-------------------------------------------------+-------------------------
 Reporter:  toralf                               |          Owner:
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_review
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |        Version:  Tor:
  TorCoreTeam201604                              |  0.2.8.1-alpha
Parent ID:                                       |     Resolution:
 Reviewer:  dgoulet                              |  Actual Points:  16
                                                 |  hours
                                                 |         Points:  medium
                                                 |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by arma):

 1)
 {{{
 \ No newline at end of file
 }}}
 for the changes file will produce surprising results when somebody cats
 all the changes files together.

 2)
 {{{
 +static int router_reachability_checks_disabled(void)
  {
    const or_options_t *options = get_options();
 }}}
 As a static helper function, it might be a bit cleaner to pass in
 {{{options}}} to it, especially since one of the two callers already has
 an 'options' hanging out ready to be passed in. We're certainly not
 consistent about this, but I think it's the righter thing to do.

 3) I like 004e1c2704 but it looks like it does two things -- first is the
 refactor, and second is the change where check_whether_orport_reachable()
 considers net_is_disabled(). "Refactor" commits are easiest to review when
 they don't also change behavior. This could become two commits, the first
 adding the net_is_disabled() to check_whether_orport_reachable(), and the
 second adding the modular function along with a note "no actual changes in
 behavior" to make it clear that it's just a refactor.

 3b) Does the behavior change from 004e1c2704 warrant a changelog entry? I
 think it probably does.

 Also, I looked through the calls to check_whether_orport_reachable() and I
 don't think that behavior change will surprise us. Good.

 3c) in control.c:
 {{{
     } else if (!strcmp(question, "status/reachability-succeeded/or")) {
       *answer = tor_strdup(check_whether_orport_reachable() ? "1" : "0");
     } else if (!strcmp(question, "status/reachability-succeeded/dir")) {
       *answer = tor_strdup(check_whether_dirport_reachable() ? "1" : "0");
     } else if (!strcmp(question, "status/reachability-succeeded")) {
       tor_asprintf(answer, "OR=%d DIR=%d",
                    check_whether_orport_reachable() ? 1 : 0,
                    check_whether_dirport_reachable() ? 1 : 0);
 }}}
 are documented as
 {{{
     "status/reachability-succeeded/or"
       0 or 1, depending on whether we've found our ORPort reachable.
     "status/reachability-succeeded/dir"
       0 or 1, depending on whether we've found our DirPort reachable.
     "status/reachability-succeeded"
 }}}
 I think the "we aren't trying to test reachability on it, so return 1"
 behavior might be surprising here. On the other hand, the reachability did
 *succeed*, in that we're not unhappy with the current state of things.
 Yuck. No need to fix it here but maybe we want to clean this up in the
 future?

 4) In commit 4dda75fc0 we end up with a new function
 decide_to_advertise_begindir(), that looks like it shares a lot of code
 with the place you cut-and-pasted it from (decide_to_advertise_dirport())?
 Leaving these two functions in place together is begging for bugs in the
 future where one gets out of sync from the other.

 5)
 {{{
 +  /* redundant - if DirPort is unreachable, we don't publish a descriptor
 */
    if (!check_whether_dirport_reachable())
 }}}
 I agree. Let's add a commit to take that check out?

 6) It seems we still have a complicated mess of similar sounding
 functions, directory-permits-this, dirport-set-that, etc. Not something
 that needs to be fixed for this ticket, but certainly something worth
 working on for the future!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18616#comment:22>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs