#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, TorBrowserTeam201911 | Actual Points:
Parent ID: | Points: .25
Reviewer: | Sponsor:
----------------------------------------------+----------------------------
Comment (by sisbell):
Replying to [comment:3 sysrqb]:
> {{{
> 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.
Good catch, I missed that one. WriteAddress with all the host support will
come in another commit. Its a minor breaking change as we will need to
change the field params for each host/port pair.
>
> {{{
> 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`
Yes that will look cleaner.
>
>
> `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`?
I believe you made this comment before, so I put in the following fix. Its
in the commit link at top, just under the issue description.
{{{
if (port != null && port >= 0) {
buffer.append(port);
} else {
buffer.append("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)`.
This was a bit of a shortcut on my part. I think the correct solution
would be to take varargs on the writeLine and just loop the append. I'll
get that fixed.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32516#comment:5>
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
