This actually looks reasonably solid. Here are a few points we should think about:
Does it actually work to only start Tor when Tor receives a SocksPort or ControlPort request? When Tor first starts after significant downtime, it needs to download a pretty big amount of directory data, and build enough circuits for user traffic. Does that happen fast enough to answer the request that made systemd launch Tor?
A Tor is supposed to actually turn itself nearly off when it sees no user traffic. Does your need for this feature mean that feature is not working?
If this is incompatible with hibernation, shouldn't we detect its use along with hibernation, and warn the user?
It appears that we do systemd socket discovery by default, unconditionally. Can that be right?
I would like if the implementation in Tor (that is, the parts outside of the sd-daemon.c code) were not systemd-specific. For example, it would also be nice to have the ability to write a little launcher program that bound to some low ports, then did a setuid(tor-daemon) and exec()d Tor. That program might want to pass a list of fds via the command line. For such a program, it would make sense to reuse 90% of this code, but have the fds come from some source other than sd-daemon.c.
There are no unit tests for this code at all. Is there anything we can do to make this tested? I would love for 0.2.5 to be the release where we stop adding untested code.
Minor nitpicks:
The new C code in Tor doesn't follow K&R indentation style like the rest of Tor.
I bet when we go to compile this on windows, we'll find at least one or two more methods that should have been wrapped in #ifndef _WIN32.
I can look at the code in more detail later, but I figured a rapid review here would be of benefit.
Thanks. It wasn't meant to be ready for merging yet, but it works well enough for me and I wanted to get some feedback before I invest more time into it.
When Tor first starts after significant downtime, it needs to download a pretty big amount of directory data, and build enough circuits for user traffic. Does that happen fast enough to answer the request that made systemd launch Tor?
I tried it by deleting /var/lib/tor and running "time torify wget http://httpbin.org/ip -o/dev/null -O-"
Over five tries I got 27 seconds, 11s, 9s, 67s, 12s. I guess some applications might hit timeouts at 67 seconds, but wget and Firefox don't seem to have a problem.
If this is incompatible with hibernation, shouldn't we detect its use along with hibernation, and warn the user?
Well it should be fixed one way or another before merging. As mentioned in the mailing list thread, we could keep the listening sockets open, but simply reject new incoming connections.
It appears that we do systemd socket discovery by default, unconditionally. Can that be right?
I think that's fine. If it isn't started by systemd then the environment vars will be unset and it will behave just like before.
For example, it would also be nice to have the ability to write a little launcher program that bound to some low ports, then did a setuid(tor-daemon) and exec()d Tor
The interface used by systemd to pass in sockets is pretty generic (sets 2 environment vars, LISTEN_PID and LISTEN_FDS). I think it could be re-used for that. TODO: documentation.
Is there anything we can do to make this tested?
Sure, I'll try to figure something out.
Trac: Username: intgr Priority: normal to minor Status: needs_review to assigned Owner: cypherpunks to intgr
So looking at the patch (quickly), I have a few remark :
the use and inclusion of sd-daemon.c is deprecated. I would also recommend against bundling if possible.
in systemd_adopt_socket, why not use the sd_is_socket function ?
in connection_listener_new, why do you close the tor_close_socket if listen fail ( ie, this is not present in the original code, and that's already done in the cleanup label code, after the label 'err')
Otherwise, that's quite a interesting approach ( comparing the exiting config with the systemd socket ), so I will take a bit more time to look in more details.