On Wed, Feb 17, 2010 at 7:56 PM, Graham Leggett <minf...@sharp.fm> wrote:
> On 17 Feb 2010, at 8:54 PM, Jeff Trawick wrote:
>
>> The daemon-report-pidfile-error situation seems to be easily handled
>> just by reversing the order of the apr_proc_detach block with the
>> if-pidfile block. Then the unusable pidfile path error could be
>> reported to the console in conjunction with -d just like a bogus cache
>> path or such errors (which are already fatal).
>
> In the process you end up logging the pid of the parent process, the process
> doomed to exit on daemonisation, and not the child process, which is the pid
> you want to keep track of.
whoops :(
> One option is to open the file before the daemonisation, and write the pid
> file after the daemonisation, assuming this kind of thing is portable, I
> don't know.
simply logging it twice in the case of daemonization should avoid any
odd behavior as well as allow reporting almost all error scenarios to
the console; that also requires closing the open pidfile handle before
daemonizing to avoid a race between unlink in the parent and creation
in the child; the attached patch seems to work
I still think exit() is in order after failing to write the pidfile,
even when there's nowhere to report an error logging the pid. There
are pros and cons, but in the interest of scripts that monitor or
terminate the daemon it seems best that no pidfile means no active
daemon.
Index: support/htcacheclean.c
===================================================================
--- support/htcacheclean.c (revision 911345)
+++ support/htcacheclean.c (working copy)
@@ -758,6 +758,28 @@
option));
}
+static void log_pid(apr_pool_t *pool, const char *pidfilename, apr_file_t
**pidfile)
+{
+ apr_status_t status;
+ char errmsg[120];
+ pid_t mypid = getpid();
+
+ if (APR_SUCCESS == (status = apr_file_open(pidfile, pidfilename, APR_WRITE
+ | APR_CREATE | APR_TRUNCATE | APR_DELONCLOSE,
+ APR_UREAD | APR_UWRITE | APR_GREAD, pool))) {
+ apr_file_printf(*pidfile, "%" APR_PID_T_FMT APR_EOL_STR, mypid);
+ }
+ else {
+ if (errfile) {
+ apr_file_printf(errfile,
+ "Could not write the pid file '%s': %s"
APR_EOL_STR,
+ pidfilename,
+ apr_strerror(status, errmsg, sizeof errmsg));
+ }
+ exit(1);
+ }
+}
+
/*
* main
*/
@@ -769,10 +791,11 @@
apr_pool_t *pool, *instance;
apr_getopt_t *o;
apr_finfo_t info;
+ apr_file_t *pidfile;
int retries, isdaemon, limit_found, intelligent, dowork;
char opt;
const char *arg;
- char *proxypath, *path, *pidfile;
+ char *proxypath, *path, *pidfilename;
char errmsg[1024];
interrupted = 0;
@@ -788,7 +811,7 @@
intelligent = 0;
previous = 0; /* avoid compiler warning */
proxypath = NULL;
- pidfile = NULL;
+ pidfilename = NULL;
if (apr_app_initialize(&argc, &argv, NULL) != APR_SUCCESS) {
return 1;
@@ -917,10 +940,10 @@
break;
case 'P':
- if (pidfile) {
+ if (pidfilename) {
usage_repeated_arg(pool, opt);
}
- pidfile = apr_pstrdup(pool, arg);
+ pidfilename = apr_pstrdup(pool, arg);
break;
} /* switch */
@@ -961,28 +984,26 @@
}
baselen = strlen(path);
+ if (pidfilename) {
+ log_pid(pool, pidfilename, &pidfile); /* before daemonizing, so we
+ * can report errors
+ */
+ }
+
#ifndef DEBUG
if (isdaemon) {
apr_file_close(errfile);
+ errfile = NULL;
+ if (pidfilename) {
+ apr_file_close(pidfile); /* delete original pidfile only in parent
*/
+ }
apr_proc_detach(APR_PROC_DETACH_DAEMONIZE);
+ if (pidfilename) {
+ log_pid(pool, pidfilename, &pidfile);
+ }
}
#endif
- if (pidfile) {
- apr_file_t *file;
- pid_t mypid = getpid();
- if (APR_SUCCESS == (status = apr_file_open(&file, pidfile, APR_WRITE
- | APR_CREATE | APR_TRUNCATE | APR_DELONCLOSE,
- APR_UREAD | APR_UWRITE | APR_GREAD, pool))) {
- apr_file_printf(file, "%" APR_PID_T_FMT APR_EOL_STR, mypid);
- }
- else if (!isdaemon) {
- apr_file_printf(errfile,
- "Could not write the pid file '%s': %s" APR_EOL_STR,
- pidfile, apr_strerror(status, errmsg, sizeof errmsg));
- }
- }
-
do {
apr_pool_create(&instance, pool);
