Refactor build_tor_connection

[lukaslueg]

It was discussed to give torstate.build_tor_connection() some refactoring in order to prevent crufty code in clients.

Consider build_tor_connection() accepting various objects as first parameter:

• Any instance of twisted.internet.endpoints
• A tuple of (reactor:host:port) from which build_tor_connection() builds a new TCP4ClientEndpoint
• A tuple of (reactor:path) from which build_tor_connection() builds a new UNIXClientEndpoint

Also consider a new function torstate.build_local_tor_connection() which will try to connect via socket first and by tcp if the control socket is not there.

The argument 'endpoint' in build_tor_connection() is changed to 'connection' to reflect the change.

def build_tor_connection(connection, build_state=True, password=None):
    if isinstance(connection, twisted.internet.endpoints.yaddayadda):
        endpoint = connection
    elif isinstance(connection, tuple):
        if len(connection) == 2:
            reactor, socket = connection
            if os.stat(socket).st_mode & (stat.S_IRGRP | stat.S_IRUSR | stat.S_IROTH):
                endpoint = UNIXClientEndpoint(reactor, socket)
            else:
                raise ValueError("Can't use '%s' as a socket" % (socket, ))
        elif len(connection) == 3:
            endpoint = TCP4ClientEndpoint(*connection)
        else:
            raise ValueError("Expected either a (reactor, socket)- or a (reactor, host, port)-tuple for argument 'connection'")
    else:
        raise ValueError("Expected instance from twisted.internet.endpoints or tuple for argument 'connection'")
    d = endpoint.connect(TorProtocolFactory(password=password))
    if build_state:
        d.addCallback(_build_state)
    else:
        d.addCallback(_wait_for_proto)
    return d

def build_local_tor_connection(reactor, host='127.0.0.1', port=9051, socket='/var/run/tor/control', **args):
    if os.path.exists(socket):
        try:
            return build_tor_connection((reactor, socket), **args)
        except:
            pass
    return build_tor_connection((reactor, host, port), **args)

Any comments?

[meejah]

One thing to bear in bind is APAF and OONI are using this method, AFAIK, so we should keep backwards compatibility ideally for their use-cases (the above looks fine for that).

Also instead of isinstance() it's probably more consistent with Twisted to check if the object implements the correct interface, like "if zope.interface.implements(connection, IStreamClientEndpoint): IStreamClientEndpoint(connection).connect(TorProtocolFactory())". I just made that syntax up for the idea :)

[meejah]

...but yes, I like this! Will make the examples more concise.

[lukaslueg]

I've created a new branch buildtorconnection that changes build_tor_connection() and adds a build_local_tor_connection(). I've changed examples/attach_stream_by_country to use the new function (most of the changes in the example are again pep8 and one bug :-)).

The documentation in torstate was not changed yet.

Should the APAF- and OONI-people have a brief look on that? All the unittests continue to work unchanged and as long as build_tor_connection is not used with keyword-arguments, they are fine.

[lukaslueg]

Yes, it's actually checking IStreamClientEndpoint.providedBy() which should be fine.

Also notice that build_local_tor_connection() can be supplied with a password-argument and such which pass through **kwargs.

[lukaslueg]

Should built_tor_connection and build_local_tor_connection return a reference to the endpoint that might have been created?

endpoint, d = build_local_tor_connection()
# and
_, d = build_tor_connection(endpoint)
endpoint, d = build_tor_connection(('localhost', 9051))

[meejah]

I'd say probably not; any code that wants this can get it from "protocol.transport.addr" (or state.protocol.transport.addr)

[lukaslueg]

closed?

[meejah]

Thanks for the patch, and closing these.