[tor-bugs] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled
Tor Bug Tracker & Wiki
blackhole at torproject.org
Thu Apr 28 02:29:55 UTC 2016
#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, TorCoreTeam201605 | 0.2.8.1-alpha
Parent ID: | Resolution:
Reviewer: dgoulet, arma | Actual Points: 20
| hours
| Points: medium
| Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):
* reviewer: dgoulet => dgoulet, arma
* actualpoints: 16 hours => 20 hours
Comment:
Replying to [comment:22 arma]:
> 1)
> {{{
> \ No newline at end of file
> }}}
> for the changes file will produce surprising results when somebody cats
all the changes files together.
A1: 2c9b85d fixup! Changes file for #18616
Also fixes a typo in the changes file name, and rewords one of the changes
to make it clearer.
> 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.
It also makes it slightly easier to unit test the function.
A2: a12df41 Refactor common code out of reachability checks
> 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.
A3:
0a4ac3e Reverts original commit 004e1c2, so I can split it up without a
force push
00bb4d7 Avoid checking ORPort reachability when the network is disabled
a12df41 Refactor common code out of reachability checks
> 3b) Does the behavior change from 004e1c2704 warrant a changelog entry?
I think it probably does.
A3b: 00bb4d7 Avoid checking ORPort reachability when the network is
disabled
> 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?
#18851 contains a branch with a control spec patch to clarify this
behaviour
> 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.
A4: 4e0e555 Refactor DirPort & begindir descriptor checks
The logic is slightly more complex now, but I think it's better than the
alternatives:
* duplicate code,
* a macro that expands to the function body
* a third variable in the refactored function indicating either a DirPort
or begindir check
> 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?
A5: 1416a28 Remove redundant descriptor checks for OR/Dir reachability
> 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!
A6: #18918 Clarify directory and ORPort checking functions
This code passes the unit tests and make test-network-all.
If you don't want to re-review commits that have stayed the same, the
unsquashed version is in bug18616-v3
When nickm wants to merge:
bug18616-v3-squashed removes commit 004e1c2 and its revert 0a4ac3e, and
rewords the commit message on 4dda75f to remove a redundant sentence
bug18616-v3-rebased is on the latest maint-0.2.8, but there were no
conflicts in the rebase
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18616#comment:24>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list