shibd should properly daemonize itself
Description
Environment
Attachments
- 05 Jun 2009, 09:01 AM
- 02 Jun 2009, 10:54 AM
- 29 May 2009, 10:47 AM
- 12 May 2009, 01:44 PM
Activity
Scott Cantor June 23, 2009 at 12:47 PM
Closing after releases.
Scott Cantor June 6, 2009 at 4:51 PM
It's fixed. I don't have firm plans at this point but if it would help to tag something, I can.
Ferenc Wágner June 6, 2009 at 4:19 PM(edited)
I'm afraid it won't even compile without replacing the unadorned pid variable with a getpid() call like this:
Index: shibd/shibd.cpp
===================================================================
— shibd/shibd.cpp (revision 3035)
+++ shibd/shibd.cpp (working copy)
@@ -366,7 +366,7 @@
if (pidfile) {
FILE* pidf = fopen(pidfile, "w");
if (pidf) {
fprintf(pidf, "%d\n", pid);
+ fprintf(pidf, "%d\n", getpid());
fclose(pidf);
}
else {
I hope I'll get around to test the current SVN next week. Or do you plan to tag RCs?
Scott Cantor June 5, 2009 at 3:20 PM
I applied your latest cleanup as is, it looks fine to me.
I don't want to change the background thread behavior because I prefer to leave the plugins free to ignore these kinds of considerations, it makes threaded code simpler to write. It's more than just that background thread anyway, since potentially any plugin could have one if it chose, and there's also the question of forking after connections are made to databases anyway, so this works best for me.
Thanks for your assistance getting this cleaned up.
Ferenc Wágner June 5, 2009 at 9:01 AM(edited)
Yes, you nailed it pretty much. Of course right now term() isn't used, but why not have it...
Interestingly, you saw that the pid file writing could stay in the child code after this separation, but still moved it into the parent. Either is good.
What I have to offer now is some cleanup. Part of it is moving pid file writing back into the child, but that's only for making my point below cleaner. So cleanup.patch basically merges all if (damonize) blocks into a single one between listener initialization and run. If only starting the StorageService cleanup thread could be delayed somehow until after init(), we could move fork() here as well and do away with the USR1 signal handling altogether, as setsid() and chdir("/") aren't expected to fail, really. So all the daemon stuff could be abstracted into an if (daemonize) do_daemonize(); or similar.
The slight change in the error message after run() is because an error can happen after successfully serving millions of requests.
Moving init() earlier also serves for better using stderr before it's reopened as /dev/null.
Otherwise the (untested) cleanup.patch should be a no-op, please use parts of it as you see fit.
The initial problem sparking this effort was that even configuration
errors were not propagated back to the init script. Fixig that is
possible by modifying shibd.cpp alone. However, one shouldn't assume
that the ListenerService initialization is always successful, as
socket creation and binding may fail as well.
Such errors are logged by the standard mechanism, but at least their
presence should be signalled to the top level. In the attached patch
this is achieved by employing the USR1 signal, which is rather alien
in its environment, but was readily possible without changing the
interface, like for example extending the shutdown flag.
If it would be possible to delay the creation of the StorageService
cleanup thread, the whole daemonization dance could be consolidated
into a single function invoked right before running the listener.
The RedHat init script should also be modified to accomodate the
new shibd behaviour. This may be as easy as removing the &.