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

Re: [tor-bugs] #9971 [Tor]: for_discovery option in add_an_entry_guard() is confusingly named

#9971: for_discovery option in add_an_entry_guard() is confusingly named
------------------------+--------------------------------------
     Reporter:  arma    |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  minor   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  tor-client easy refactor
Actual Points:          |  Parent ID:
       Points:          |
------------------------+--------------------------------------

Comment (by asn):

 Replying to [comment:5 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()`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9971#comment:7>
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