[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #27490 [Core Tor/Tor]: When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for a directory or orport connection, prefer IPv4 or IPv6 at random
#27490: When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for
a directory or orport connection, prefer IPv4 or IPv6 at random
--------------------------+--------------------------------
Reporter: neel | Owner: neel
Type: enhancement | Status: needs_revision
Priority: Medium | Milestone:
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: #17835 | Points:
Reviewer: teor | Sponsor:
--------------------------+--------------------------------
Changes (by teor):
* status: needs_review => needs_revision
Comment:
I am very confused by this branch, and I'm not sure if I can review it
properly.
It seems to have:
* one commit that makes most of the changes
* three commits that make some log and comment changes
* the same three commits that make some log and comment changes. Please
don't add duplicate commits to a branch.
* a merge
* a bunch of extra commits that choose IP addresses randomly, but in
different places all through the code
* a commit that fixes the unit tests
Some of this is my fault, because I asked you to add ClientAutoIPv6ORPort,
but then I talked about ClientPreferIPv6ORPort in the rest of my feedback.
Here's what this patch needs to do:
* ClientPreferIPv6ORPort is an existing option, this patch must not change
what it does
* ClientAutoIPv6ORPort is a new option, it can do new things
Here's how we can move forward:
* please open a new pull request like commit 1a98526, but using
ClientAutoIPv6ORPort, rather than changing ClientPreferIPv6ORPort
* please add the logging changes from commit 2311a42
* please add new unit tests for ClientAutoIPv6ORPort (if the unit tests
for ClientPreferIPv6ORPort fail, then you have changed how
ClientPreferIPv6ORPort works)
Adding all those extra comments isn't as helpful as I thought it would be.
Let's just change the function comment on
fascist_firewall_prefer_ipv6_orport().
I am not sure why the 4 "assign it a random preference" commits are
needed.
I think it is enough for fascist_firewall_prefer_ipv6_orport() to return a
random value when ClientAutoIPv6ORPort is 1.
Why do we need to choose at random in these places as well?
But we might need to change rewrite_node_address_for_bridge() so that we
choose a random address when ClientAutoIPv6ORPort is 1.
But that is a simple change, because the code already calls
fascist_firewall_prefer_ipv6_orport().
All we need to do it change:
"if (options->ClientPreferIPv6ORPort == -1) {"
to
"if (options->ClientPreferIPv6ORPort == -1 &&
options->ClientAutoIPv6ORPort == 0) {"
Then, when ClientAutoIPv6ORPort is 1, the code will choose at random.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27490#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