Make Write Methods Clearer in TorConfigBuilder
Make Write Methods Clearer in TorConfigBuilder. We have a lot of duplicate buffer appends that we can cleanup to make code more readable.
Activity
Ready for Review. No breaking API changes.
Changes
- Added write line and write properties methods and used throughout property settings
- Did a format on the code to clear up previously flag nits
- I added a write address method. Its currently unused now but will be used in subsequent commits
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/1f95271ae222839797825020a41491ac8aab0d8f
Trac:
Status: new to needs_reviewpublic 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
writeAddressseems 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)insteadpublic 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 andtransportPluginMeekinto a singletransportPluginand useClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit execwriteAddress+ if (port != null) { + buffer.append(port <= 0 ? "auto" : port); + } else { + buffer.append("auto"); + }I'm still not sure we should change
port <= 0intoauto.*Port 0is already defined as disabling that type of port. Do we need two ways of settingauto?+ 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).Trac:
Status: needs_review to needs_revisionReplying to 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;
-
} }}} nit: This could use
return writeLine("SocksListenAddress 0.0.0.0");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) -
} }}} This can be simplified to
.writeLine("ClientTransportPlugin obfs4 exec", clientPath);ClientTransportPlugin obfs3,obfs4 exec. Or, if we take this two steps further, combine this method andtransportPluginMeekinto a singletransportPluginand useClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit execYes 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 <= 0intoauto.*Port 0is already defined as disabling that type of port. Do we need two ways of settingauto?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.-
Changes:
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/75491165af6339f28503eae09b1984632df78b0c
- Use writeAddress for orPort. SocksListenAddress doesn't take a port (as far as I can tell) so I can't use the writeAddress method for it as this would add ':auto' constant to the field.
- ClientTransportPlugin loads meek_lite, obf3 and obfs4 on same line. The implementation will write out all of the supported bridges from settings onto a single line. This assumes that all bridges can be handled by one clientTransportPlugin. This is suitable for the Android implementation but we should revisit this issue later.
- Use varargs for writeLine method so we can add as many values as we like
- Changed writeTrueProperty and writeFalseProperty to use writeLine
- The auto port issue was fixed in previous commit (and is also in this commit). Port will be "auto" if null or if port value is less than 0.
Open Issues:
- SocksListenAddress is deprecated. So need to investigate removal/migration of the field. This needn't be addressed in this commit.
Trac:
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added
Status: needs_revision to needs_review
