for_discovery option in add_an_entry_guard() is confusingly named

In #9946 (moved) 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.

changed milestone to %Tor: 0.2.8.x-final

added 027-triaged-1-in 028-triaged component::core tor/tor easy milestone::Tor: 0.2.8.x-final points::small pre028-patch priority::very low refactor resolution::wontfix severity::normal sponsor::U status::closed tor-client type::defect version::tor 0.2.7 labels

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.???
Keywords: tor-client deleted, tor-client easy refactor added

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?

Trac:
Username: merge

Trac:
Status: new to needs_review

Trac:
Username: merge

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

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

Trac:
Username: merge

Trac:
Username: merge

0002-rename-for_discovery-argument-of-add_an_entry_guard-.patch

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'.

Trac:
Username: merge

Trac:
Username: merge

0003-remove-prepend-argument-of-add_an_entry_guard.patch

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.

Trac:
Username: merge

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

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.

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.

Trac:
Username: merge

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 (moved) and #12202 (moved)). The next guard refactoring that needs to happen is #12595 (moved) 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().

Trac:
Status: needs_review to needs_revision

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.7.x-final

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

Trac:
Keywords: tor-client easy refactor deleted, tor-client, refactor, easy, 027-triaged-1-in added

Trac:
Points: N/A to small
Version: N/A to Tor: 0.2.7
Priority: minor to trivial
Keywords: N/A deleted, SponsorU added

Trac:
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.8.x-final

Trac:
Keywords: N/A deleted, 028-triaged added

Bulk-replace SponsorU keyword with SponsorU field.

Trac:
Keywords: SponsorU deleted, N/A added
Sponsor: N/A to SponsorU

Trac:
Keywords: N/A deleted, pre028-patch added

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

Trac:
Severity: N/A to Normal
Status: needs_revision to closed
Resolution: N/A to wontfix

closed

moved to tpo/core/tor#9971 (closed)