At 06:25 PM 3/27/2003, you wrote:
>Cool beans... Some comments:
>
>> - else if (strcEQ(arg, "sysvsem")) {
>> + else if (!strcasecmp(meth, "sysvsem") && file) {
>
>We should not require that sysvsem lists a file..
Agreed.
>> #if APR_HAS_FLOCK_SERIALIZE
>> + else if (!strcasecmp(meth, "file") && file) {
>> mc->nMutexMech = APR_LOCK_FLOCK;
>> + }
>> +#elif APR_HAS_FCNTL_SERIALIZE
>> + else if (!strcasecmp(meth, "file") && file) {
>> mc->nMutexMech = APR_LOCK_FCNTL;
>
>This imples that, with file, flock is prefered over fcntl. These should be
>reversed to match the ordering everywhere else :)
Agreed.
>> #if APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM)
>> + else if (!strcasecmp(meth, "sem")) {
>> mc->nMutexMech = APR_LOCK_SYSVSEM;
>> + }
>> +#elif APR_HAS_POSIXSEM_SERIALIZE
>> + else if (!strcasecmp(meth, "sem")) {
>> mc->nMutexMech = APR_LOCK_POSIXSEM;
>
>These should be reversed as well, so that posix is prefered over sysvsem
I've made it so. Thanks for all the careful review.
In fixing the code, it became even easier to read... See the attached
third revision of my two SSL patches or the last commit to 2.1-dev.
Bill
Index: modules/ssl/ssl_engine_config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_config.c,v
retrieving revision 1.73
retrieving revision 1.76
diff -u -r1.73 -r1.76
--- modules/ssl/ssl_engine_config.c 23 Feb 2003 17:12:43 -0000 1.73
+++ modules/ssl/ssl_engine_config.c 28 Mar 2003 00:43:26 -0000 1.76
@@ -368,11 +368,20 @@
const char *ssl_cmd_SSLMutex(cmd_parms *cmd,
void *dcfg,
- const char *arg)
+ const char *arg_)
{
const char *err;
SSLModConfigRec *mc = myModConfig(cmd->server);
-
+ /* Split arg_ into meth and file */
+ char *meth = apr_pstrdup(cmd->temp_pool, arg_);
+ char *file = strchr(meth, ':');
+ if (file) {
+ *(file++) = '\0';
+ if (!*file) {
+ file = NULL;
+ }
+ }
+
if ((err = ap_check_cmd_context(cmd, GLOBAL_ONLY))) {
return err;
}
@@ -380,97 +389,71 @@
if (ssl_config_global_isfixed(mc)) {
return NULL;
}
-
- if (strcEQ(arg, "none") || strcEQ(arg, "no")) {
+ if (!strcasecmp(meth, "none") || !strcasecmp(meth, "no")) {
mc->nMutexMode = SSL_MUTEXMODE_NONE;
+ return NULL;
}
+
+ /* APR determines temporary filename unless overridden below,
+ * we presume file indicates an szMutexFile is a file path
+ * unless the method sets szMutexFile=file and NULLs file
+ */
+ mc->nMutexMode = SSL_MUTEXMODE_USED;
+ mc->szMutexFile = NULL;
+
/* NOTE: previously, 'yes' implied 'sem' */
- else if (strcEQ(arg, "default") || strcEQ(arg, "yes")) {
- mc->nMutexMode = SSL_MUTEXMODE_USED;
+ if (!strcasecmp(meth, "default") || !strcasecmp(meth, "yes")) {
mc->nMutexMech = APR_LOCK_DEFAULT;
- mc->szMutexFile = NULL; /* APR determines temporary filename */
}
-#if APR_HAS_FLOCK_SERIALIZE
- else if (strlen(arg) > 6 && strcEQn(arg, "flock:", 6)) {
- const char *file = ap_server_root_relative(cmd->pool, arg+6);
- if (!file) {
- return apr_pstrcat(cmd->pool, "Invalid SSLMutex flock: path ",
- arg+6, NULL);
- }
- mc->nMutexMode = SSL_MUTEXMODE_USED;
- mc->nMutexMech = APR_LOCK_FLOCK;
- mc->szMutexFile = apr_psprintf(mc->pPool, "%s.%lu",
- file, (unsigned long)getpid());
- }
-#endif
#if APR_HAS_FCNTL_SERIALIZE
- else if (strlen(arg) > 6 && strcEQn(arg, "fcntl:", 6)) {
- const char *file = ap_server_root_relative(cmd->pool, arg+6);
- if (!file) {
- return apr_pstrcat(cmd->pool, "Invalid SSLMutex fcntl: path ",
- arg+6, NULL);
- }
- mc->nMutexMode = SSL_MUTEXMODE_USED;
+ else if ((!strcasecmp(meth, "fcntl") || !strcasecmp(meth, "file")) && file) {
mc->nMutexMech = APR_LOCK_FCNTL;
- mc->szMutexFile = apr_psprintf(mc->pPool, "%s.%lu",
- file, (unsigned long)getpid());
}
#endif
-#if APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM)
- else if (strcEQ(arg, "sysvsem")) {
- mc->nMutexMode = SSL_MUTEXMODE_USED;
- mc->nMutexMech = APR_LOCK_SYSVSEM;
- mc->szMutexFile = NULL; /* APR determines temporary filename */
+#if APR_HAS_FLOCK_SERIALIZE
+ else if ((!strcasecmp(meth, "flock") || !strcasecmp(meth, "file")) && file) {
+ mc->nMutexMech = APR_LOCK_FLOCK;
}
#endif
#if APR_HAS_POSIXSEM_SERIALIZE
- else if (strcEQ(arg, "posixsem")) {
- mc->nMutexMode = SSL_MUTEXMODE_USED;
+ else if (!strcasecmp(meth, "posixsem") || !strcasecmp(meth, "sem")) {
mc->nMutexMech = APR_LOCK_POSIXSEM;
- mc->szMutexFile = NULL; /* APR determines temporary filename */
- }
-#endif
-#if APR_HAS_PROC_PTHREAD_SERIALIZE
- else if (strcEQ(arg, "pthread")) {
- mc->nMutexMode = SSL_MUTEXMODE_USED;
- mc->nMutexMech = APR_LOCK_PROC_PTHREAD;
- mc->szMutexFile = NULL; /* APR determines temporary filename */
- }
-#endif
-#if APR_HAS_FLOCK_SERIALIZE || APR_HAS_FCNTL_SERIALIZE
- else if (strlen(arg) > 5 && strcEQn(arg, "file:", 5)) {
- const char *file = ap_server_root_relative(cmd->pool, arg+5);
- if (!file) {
- return apr_pstrcat(cmd->pool, "Invalid SSLMutex file: path ",
- arg+5, NULL);
+ /* Posix/SysV semaphores aren't file based, use the literal name
+ * if provided and fall back on APR's default if not. Today, APR
+ * will ignore it, but once supported it has an absurdly short limit.
+ */
+ if (file) {
+ mc->szMutexFile = apr_pstrdup(cmd->server->process->pool, file);
+
+ file = NULL;
}
- mc->nMutexMode = SSL_MUTEXMODE_USED;
-#if APR_HAS_FLOCK_SERIALIZE
- mc->nMutexMech = APR_LOCK_FLOCK;
-#endif
-#if APR_HAS_FCNTL_SERIALIZE
- mc->nMutexMech = APR_LOCK_FCNTL;
-#endif
- mc->szMutexFile =
- apr_psprintf(mc->pPool, "%s.%lu",
- file, (unsigned long)getpid());
}
#endif
-#if (APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM)) ||
APR_HAS_POSIXSEM_SERIALIZE
- else if (strcEQ(arg, "sem")) {
- mc->nMutexMode = SSL_MUTEXMODE_USED;
#if APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM)
- mc->nMutexMech = APR_LOCK_SYSVSEM;
-#endif
-#if APR_HAS_POSIXSEM_SERIALIZE
- mc->nMutexMech = APR_LOCK_POSIXSEM;
+ else if (!strcasecmp(meth, "sysvsem") || !strcasecmp(meth, "sem")) {
+ mc->nMutexMech = APR_LOCK_SYSVSEM;
+ }
#endif
- mc->szMutexFile = NULL; /* APR determines temporary filename */
+#if APR_HAS_PROC_PTHREAD_SERIALIZE
+ else if (!strcasecmp(meth, "pthread")) {
+ mc->nMutexMech = APR_LOCK_PROC_PTHREAD;
}
#endif
else {
- return apr_pstrcat(cmd->pool, "Invalid SSLMutex argument ",
- arg, " (", ssl_valid_ssl_mutex_string, ")", NULL);
+ return apr_pstrcat(cmd->pool, "Invalid SSLMutex argument ", arg_,
+ " (", ssl_valid_ssl_mutex_string, ")", NULL);
+ }
+
+ /* Unless the method above assumed responsibility for setting up
+ * mc->szMutexFile and NULLing out file, presume it is a file we
+ * are looking to use
+ */
+ if (file) {
+ mc->szMutexFile = ap_server_root_relative(cmd->server->process->pool, file);
+ if (!mc->szMutexFile) {
+ return apr_pstrcat(cmd->pool, "Invalid SSLMutex ", meth,
+ ": filepath ", file, NULL);
+ }
}
return NULL;
Index: modules/ssl/ssl_engine_mutex.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_mutex.c,v
retrieving revision 1.20
retrieving revision 1.22
diff -u -r1.20 -r1.22
--- modules/ssl/ssl_engine_mutex.c 26 Mar 2003 22:31:56 -0000 1.20
+++ modules/ssl/ssl_engine_mutex.c 27 Mar 2003 23:51:22 -0000 1.22
@@ -73,8 +73,12 @@
if (mc->nMutexMode == SSL_MUTEXMODE_NONE)
return TRUE;
+ if (mc->pMutex) {
+ return TRUE;
+ }
if ((rv = apr_global_mutex_create(&mc->pMutex, mc->szMutexFile,
- mc->nMutexMech, p)) != APR_SUCCESS) {
+ mc->nMutexMech, s->process->pool))
+ != APR_SUCCESS) {
if (mc->szMutexFile)
ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
"Cannot create SSLMutex with file `%s'",
