#32516: Make Write Methods Clearer in TorConfigBuilder -----------------------------------------------+--------------------------- Reporter: sisbell | Owner: tbb-team Type: defect | Status: | needs_revision Priority: Medium | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: tbb-mobile, TorBrowserTeam201911R | Actual Points: Parent ID: | Points: .25 Reviewer: | Sponsor: -----------------------------------------------+--------------------------- Changes (by sysrqb):
* status: needs_review => needs_revision Comment: {{{ public TorConfigBuilder makeNonExitRelay(String dnsFile, int orPort, String nickname) { - buffer.append("ServerDNSResolvConfFile ").append(dnsFile).append('\n'); - buffer.append("ORPort ").append(orPort).append('\n'); - buffer.append("Nickname ").append(nickname).append('\n'); - buffer.append("ExitPolicy reject *:*").append('\n'); - return this; + writeLine("ServerDNSResolvConfFile", dnsFile); + writeLine("ORPort", String.valueOf(orPort)); }}} nit: ORPort should take an address, as well, and `writeAddress` seems more explicit. But don't worry about that right now, we can come back to this later. {{{ public TorConfigBuilder proxyOnAllInterfaces() { - buffer.append("SocksListenAddress 0.0.0.0").append('\n'); - return this; + return writeLine("SocksListenAddress 0.0.0.0"); } }}} nit: This could use `writeAddress("SocksListenAddress", "0.0.0.0", null, null)` instead {{{ public TorConfigBuilder transportPluginObfs(String clientPath) { - buffer.append("ClientTransportPlugin obfs3 exec ").append(clientPath).append('\n'); - buffer.append("ClientTransportPlugin obfs4 exec ").append(clientPath).append('\n'); - return this; + return writeLine("ClientTransportPlugin obfs3 exec", clientPath) + .writeLine("ClientTransportPlugin obfs4 exec", clientPath); } }}} This can be simplified to `ClientTransportPlugin obfs3,obfs4 exec`. Or, if we take this two steps further, combine this method and `transportPluginMeek` into a single `transportPlugin` and use `ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec` `writeAddress` {{{ + if (port != null) { + buffer.append(port <= 0 ? "auto" : port); + } else { + buffer.append("auto"); + } }}} I'm still not sure we should change `port <= 0` into `auto`. `*Port 0` is already defined as disabling that type of port. Do we need two ways of setting `auto`? {{{ + public TorConfigBuilder writeLine(String value, String value2) { + return writeLine(value + " " + value2); + } }}} This seems like a surprising overload. I'm not opposed to having it, but it doesn't seem helpful and it is more confusing (less readable) than simply passing the string concatenation directly into `writeLine(String)`. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32516#comment:3> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs