[tor-bugs] #32516 [Applications/Tor Browser]: Make Write Methods Clearer in TorConfigBuilder

[Tor Bug Tracker & Wiki]

#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