Daemon should use event loop

[missionsix]

Background

---

I've been interested in improving the transmission daemon source due to recent issues I've had with watchdir on linux. I've spent some time now looking at daemon.c/watch.c and realized there's a couple optimizations that can be done.

In order to cleanup the notify stuff, I've had to first fix the daemon loop.

Proposed Solution

---

I'd like to replace the main loop with an event loop and timer which allows for further development using libevent as the threading model in the daemon.

For example, I can register an event socket watching the inotify FD for changes, and instrument a callback to add new torrents when they are detected in the target directory. This reduces the overhead of read()-ing the inotify fd every 1 second, and allows for edge triggered updates.

Other optimizations could be made in the daemon with the event loop including safer signal handling, SIGHUP updating, watching for settings file changes, etc.

[missionsix]

Daemon eventloop patch

[mike.dld]

I'm not exactly proficient with libevent yet, but the patch looks nice to me. I have some comments though:

1. event_new() may return NULL and event_add() may return -1. Both are fatal in current patch version (event_add() passed NULL would crash, event_base_dispatch() with no pending events would return 1 immediately) and should be handled accordingly.
2. Check for ev_base validity during teardown is not needed, already handled during initialization.
3. Taking in account p.1, check for status_ev validity during teardown should also be unnecessary.
4. tr_wait_msec(1000) in reportStatus() is not needed (looks like refactoring leftover).
5. As we're striving for optimization, I'd also wrap systemd status reporting in reportStatus() into #ifdef USE_SYSTEMD_DAEMON to disable meaningless calls to tr_sessionGetRawSpeed_KBps() (although not the point of this patch at all).

[missionsix]

I've attached an updated version with some of the feedback addressed.

I'm not sure I like #3, given that things should be properly shut down and not just exit(1), therefore I added the cleanup: label which should be used as the exit point. This allows things like inotify table to be properly run down, such that there are no leaks.

Good catch on #4, I did mean to remove that.

I think #5 should be addressed in another patch.

[mike.dld]

Don't see the updated patch, please try uploading it under different name.

[missionsix]

version 2 of the daemon event loop changes

[jordan]

v2 of the patch looks pretty good.

One trivial readability change I'd suggest -- one wouldn't expect a function named reportStatus() to have a side-effect of pumping the watchdir code. It would be less surprising if the libevent's callback func was a wrapper function named (say) periodicUpdate() that called both dtr_watchdir_update() and reportStatus().

That would also be slightly cleaner for Mike's fifth suggestion, as reportStatus()'s body could be conditionally compiled iff USE_SYSTEMD_DAEMON is #defined.

[jordan]

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

[mike.dld]

Replying to jordan:

... one wouldn't expect a function named reportStatus() to have a side-effect of pumping the watchdir code ...

I decided not to treat this as an issue since missionsix said the idea was not to call dtr_watchdir_update() periodically (at least with inotify), so I thought some other patch would follow which would extract this call out to separate function anyway (and I was right, as there is #5663, although it still has pumpLogMessages() inside reportStatus()).

[mike.dld]

Replying to jordan:

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

Looks good to me.

[missionsix]

Replying to jordan:

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

Nope, LGTM.

[jordan]

v3 diff added to trunk in r14265

[craig]

I think this patch isn’t working correctly for the DLink DNS-325 storage unit. I’ve been running Transmission-2.82 for a while without any issues.

When I upgraded to Transmission-2.83 via slacker, I receive the following error.

/ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

I thought it was just a path or env error but the issue is beyond my skillset. I rolled back to Transmission-2.82 and everything works fine.

Below is some info for a more knowledgeable person to possible troubleshoot for a future release.

---

## /ffp/bin/funpkg -u /ffp/funpkg/cache/kylek/Transmission-2.83-arm-1.txz

Scanning /ffp/funpkg/cache/kylek/Transmission-2.83-arm-1.txz ... Upgrading to Transmission-2.83-arm-1 Removing Transmission-2.82-arm-1,upgraded,20140720_104741

Searching for *.new files: /ffp/etc /ffp/start /ffp/etc/examples/lighttpd.conf-with-php.new /ffp/etc/examples/lighttpd.conf.new /ffp/etc/ssh/sshd_config.new /ffp/etc/ssh/moduli.new /ffp/etc/ssl/openssl.cnf.new /ffp/start/kickwebs.sh.new /ffp/start/rsyncd.sh.new /ffp/start/sshd.sh.new /ffp/start/kickangel.sh.new /ffp/start/lighttpd.sh.new Done.

## ./transmission.sh start Starting transmission-daemon /ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

## echo $PATH PATH=/opt/sbin:/opt/bin:/ffp/sbin:/usr/sbin:/sbin:/ffp/bin:/usr/bin:/bin:/opt/lib

## locate libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5.1.8 /opt/lib/libevent-2.0.so.5 /opt/lib/libevent-2.0.so.5.1.8

[mike.dld]

Replying to craig:

/ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

This has nothing to do with code changes. It's your environment issue.

Replying to craig:

## echo $PATH PATH=/opt/sbin:/opt/bin:/ffp/sbin:/usr/sbin:/sbin:/ffp/bin:/usr/bin:/bin:/opt/lib

## locate libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5.1.8 /opt/lib/libevent-2.0.so.5 /opt/lib/libevent-2.0.so.5.1.8

On *NIX systems, paths where dynamic linker looks for libraries doesn't include PATH environment variable (you can see the libraries dynamic linker is currently aware of by executing ldconfig -v). Try running transmission-daemon manually, adding path to libraries explicitly beforehand:

# export LD_LIBRARY_PATH=/opt/lib:$LD_LIBRARY_PATH
# /ffp/bin/transmission-daemon -f

(You may need to use setenv or something else instead of export depending on your system). If that helps, modify either dynamic linker configuration (/etc/ld.so.conf or similar) or transmission startup script to include that path above.

[mike.dld]

Replying to mike.dld:

If that helps, modify either dynamic linker configuration (/etc/ld.so.conf or similar) or transmission startup script to include that path above.

I'm also not sure whether "/mnt/HD/HD_a2/ffp/opt/optware" or "/opt" are standard install prefixes on your system. If those aren't, you may as well just install libevent normally (with "/ffp" prefix? transmission-daemon is installed there for some reason) and it would work as is.

[craig]

Thanks for the help. The dynamic library development class is coming back into focus.

I rule out my DNS-325 by building a clean OS and booting from a USB stick. I tried the suggestions from the above comments. None of them worked. However when I installed the libevent package via funpkg the issue was resolved.

I agree there is nothing wrong with the code base. It appears to be an issue the way KyleK packaged up Transmission 2.83.