Opened 6 years ago

Closed 4 years ago

#9971 closed defect (wontfix)

for_discovery option in add_an_entry_guard() is confusingly named

Reported by: arma Owned by:
Priority: Very Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Normal Keywords: tor-client, easy, refactor, 027-triaged-1-in, 028-triaged, pre028-patch
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor: SponsorU

Description

In #9946 I added a new argument "for_discovery" to add_an_entry_guard(). Nick prefers "provisional" or "probationary".

In parallel, I think we should probably rename the made_contact field in entry guard t, to be *why* we're remembering that we've made contact, rather than simply that we have.

And lastly, we should do something about the godawful number of int arguments that add_an_entry_guard() now takes.

Child Tickets

Attachments (3)

0001-rename-entry_guard_t-s-made_contact-to-used_so_save_.patch (6.2 KB) - added by merge 5 years ago.
rename the bit made_contact in entry_guard_t to used_so_save_if_down to make the process more readable.
0002-rename-for_discovery-argument-of-add_an_entry_guard-.patch (1.9 KB) - added by merge 5 years ago.
rename the int argument for_discovery of add_an_entry_guard() to forced_probationary. It resembles a, in some sense, not optimal (forced) situation regarding a desired 'level of anonymity'.
0003-remove-prepend-argument-of-add_an_entry_guard.patch (3.2 KB) - added by merge 5 years ago.
node_t *chosen is a node to add prepend is set if the guard should become first in the list there are 2 users of add_an_entry_guard() that pass it a chosen node. One is a bridge (prepend) and the other one is a user-defined node (!prepend), so: Given the fact that the list is not supposed to be long and the two 'users' are somewhat similar, prepend the node if explicitly given.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by nickm

Keywords: easy refactor added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:2 Changed 5 years ago by merge

I proposed appropriate changes on tor-dev: https://lists.torproject.org/pipermail/tor-dev/2014-June/007059.html


edit: why not put the changes in here as well: I append seperate patches for the 3 sub-issues mentioned. They apply seperately to the current tree, so a maintainer can
quickly pick one of them up, if happy with it.

There shouldn't be a conflict in *1* and *2* but I could append a patchset anyway.

sub-issue *3* is *only* a call for advice and will be written on top of
the other ones, after discussion.

1* rename entry_guard_t's made_contact to used_so_save_if_down

I think that's readable. Is that about what arma had in mind?

2* rename for_discovery argument of add_an_entry_guard().

I like probationary more than provisional. Those 2 are suggested in issue 9971.
I chose forced_probationary because isn't it strictly a suboptimal situation
in terms of desired 'grade of anonymity'. What do you think?

3* NEEDS REVIEW FIRST: regarding the int arguments of add_an_entry_guard(). I look at:

node_t *chosen is a node to add.
prepend is set if the guard should become first in the list?
for_discovery is set if the guard is to be added because no guard in the list is running.

there are 2 users of add_an_entry_guard() that pass it a chosen node. One is
a bridge (prepend) and the other one is a user-defined node (!prepend), so:

Given the fact that the list is not supposed to be long and the two 'users'
are somewhat similar, why not prepend the node if explicitly given?

Last edited 5 years ago by merge (previous) (diff)

comment:3 Changed 5 years ago by asn

Status: newneeds_review

Changed 5 years ago by merge

rename the bit made_contact in entry_guard_t to used_so_save_if_down to make the process more readable.

Changed 5 years ago by merge

rename the int argument for_discovery of add_an_entry_guard() to forced_probationary. It resembles a, in some sense, not optimal (forced) situation regarding a desired 'level of anonymity'.

Changed 5 years ago by merge

node_t *chosen is a node to add prepend is set if the guard should become first in the list there are 2 users of add_an_entry_guard() that pass it a chosen node. One is a bridge (prepend) and the other one is a user-defined node (!prepend), so: Given the fact that the list is not supposed to be long and the two 'users' are somewhat similar, prepend the node if explicitly given.

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.6.x-final

comment:5 Changed 5 years ago by nickm

asn, does this conflict with the guard refactoring stuff you're doing? If so, couple you please incorporate the first two as feasible? These patches seem like reasonably good ideas to me.

The third one makes me nervous: we should still probably be appending the stuff in EntryNodes, right? Otherwise I worry we'll change our guards too fast.

comment:6 Changed 5 years ago by merge

Good. The third one should make you nervous and I don't sign off on it at all. It doesn't even apply to the current tree (the other two still do). You can either close this ticket ignoring the many-function-arguments-problem, spend some time on it considering the recent development in Guard-management or wait quite some time for me to be able to look at it.

comment:7 in reply to:  5 Changed 5 years ago by asn

Replying to nickm:

asn, does this conflict with the guard refactoring stuff you're doing? If so, couple you please incorporate the first two as feasible? These patches seem like reasonably good ideas to me.

FWIW, the guard refactoring stuff I was doing have been merged (#12207 and #12202). The next guard refactoring that needs to happen is #12595 but this is more long term and I haven't started coding it yet.

Now, on the topic of those two patches:

  • 0001-rename-entry_guard_t-s-made_contact-to-used_so_save_.patch

I'm not sure if I like this change. While I agree that made_contact is not the best name, I'm not sure if used_so_save_if_down is more appropriate.

IIUC, that variable is used in other places too, not just for "saving it down". For example, it's used during the network down event recognition in entry_guard_register_connect_status(), and also in populate_live_entry_guards() as part of:

      /* Always start with the first not-yet-contacted entry
       * guard. Otherwise we might add several new ones, pick
       * the second new one, and now we've expanded our entry
       * guard list without needing to. */
  • 0002-rename-for_discovery-argument-of-add_an_entry_guard-.patch

I don't really understand the for_discovery feature, so I can't comment on whether forced_probationary is a better name. Judging by the comment in add_an_entry_guard() it seems that this feature is some kind of kludge, and just the variable name change doesn't make it much easier to understand (especially, if we change made_contact to used_so_save_if_down then the comment doesn't even make sense).

In any case, if you understand this feature better, and you think that force_probationary is a better name, feel free to merge it.

Also, the patch misses a space in the comment of add_an_entry_guard().

comment:8 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:9 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

comment:10 Changed 5 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:11 Changed 5 years ago by isabela

Keywords: SponsorU added
Points: small
Priority: minortrivial
Version: Tor: 0.2.7

comment:12 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:13 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:14 Changed 4 years ago by nickm

Keywords: SponsorU removed
Sponsor: SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:15 Changed 4 years ago by nickm

Keywords: pre028-patch added

comment:16 Changed 4 years ago by nickm

Resolution: wontfix
Severity: Normal
Status: needs_revisionclosed

I'm going to call this 'wontfix', since we have other work ongoing (see eg #17261, #17262) to replace the guard selection code entirely. That makes it less critical for the current code to have well-named arguments.

Note: See TracTickets for help on using tickets.