On 4/10/2012 10:31 AM, Jeff Trawick wrote:
> On Tue, Apr 10, 2012 at 12:05 AM, <[email protected]> wrote:
>> Author: wrowe
>> Date: Tue Apr 10 04:05:23 2012
>> New Revision: 1311569
>>
>> URL: http://svn.apache.org/viewvc?rev=1311569&view=rev
>> Log:
>> Introduce FcgidWin32PreventOrphans directive on Windows to use OS
>> Job Control Objects to terminate all running fcgi's when the worker
>> process has been abruptly terminated.
>>
>> [wrowe: I've changed the volume level on some error levels, moved
>> the rv into the error log schema, and moved all commentary over to
>> the actual conf assignment function. With the exception of style
>> cleanup, this remains almost entirely Thangaraj's creation. Also
>> have added simple docs.]
>>
>> PR: 51078
>> Submitted by: Thangaraj AntonyCrouse <thangaraj gmail.com>
>
> Is anyone opposed to changing the directive from _NO_ARGS to _FLAG?
Make it so.
> (a couple of more comments are below)
>
> I'm happy to do the tweaks after confirmation.
Thanks!
>>
>> + /* Cleanup the Job object if present */
>> + conf = ap_get_module_config(((server_rec*)server)->module_config,
>> + &fcgid_module);
>> +
>> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
>> + CloseHandle(conf->hJobObjectForAutoCleanup);
>> + }
>> +
>
> Isn't it more idiomatic to register a cleanup for the job object
> rather than explicitly checking for whether or not it exists in
> different code?
+1
>> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c (original)
>> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c Tue Apr 10 04:05:23
>> 2012
>> @@ -63,6 +63,7 @@ apr_status_t proc_spawn_process(const ch
>> {
>> HANDLE *finish_event, listen_handle;
>> SECURITY_ATTRIBUTES SecurityAttributes;
>> + fcgid_server_conf *sconf;
>> apr_procattr_t *proc_attr;
>> apr_status_t rv;
>> apr_file_t *file;
>> @@ -177,9 +178,31 @@ apr_status_t proc_spawn_process(const ch
>> if (rv != APR_SUCCESS) {
>> ap_log_error(APLOG_MARK, APLOG_ERR, rv, procinfo->main_server,
>> "mod_fcgid: can't run %s", wargv[0]);
>> + return rv;
>> }
We leave the new return rv, above.
>> - return rv;
>> + /* FcgidWin32PreventOrphans feature */
>> + sconf = ap_get_module_config(procinfo->main_server->module_config,
>> + &fcgid_module);
>> + if (sconf == NULL) {
>> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, procinfo->main_server,
>> + "mod_fcgid: missing server configuration record");
>> + return APR_SUCCESS;
>> + }
>
> Let's just crash here if sconf is NULL. Agreed?
+1
