httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Date Tue, 10 Apr 2012 15:55:25 GMT
On 4/10/2012 10:31 AM, Jeff Trawick wrote:
> On Tue, Apr 10, 2012 at 12:05 AM,  <wrowe@apache.org> 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


Mime
View raw message