Re: [patch] two small problems in ssl_engine_mutex.c

2003-03-31 Thread Jim Jagielski 
William A. Rowe, Jr. wrote:
> 
> One, win32 won't compile (nor any platform missing chown).  In this
> case we didn't need it and have a good macro to look at.

+1. We also use chown in a few other places as well:

./modules/generators/mod_cgid.c:if (chown(sconf->sockname, 
unixd_config.user_id, -1) < 0) {
./modules/ssl/ssl_scache_dbm.c:chown(mc->szSessionCacheDataFile, 
unixd_config.user_id, -1 /* no gid change */);
./modules/ssl/ssl_scache_dbm.c:if (chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, SSL_DBM_FILE_SUFFIX_DIR, NULL),
./modules/ssl/ssl_scache_dbm.c:if (chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, ".db", NULL),
./modules/ssl/ssl_scache_dbm.c:chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, ".dir", NULL),
./modules/ssl/ssl_scache_dbm.c:if (chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, SSL_DBM_FILE_SUFFIX_PAG, NULL),
./modules/ssl/ssl_scache_dbm.c:if (chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, ".db", NULL),
./modules/ssl/ssl_scache_dbm.c:chown(apr_pstrcat(p, 
mc->szSessionCacheDataFile, ".pag", NULL),

> 
> This raised another bug in the next line.  We assumed because we
> default to SYSV mutexes we should do that magic.  I believe this is
> wrong, and we should be looking for that sort of lock explicitly.
> 

+1. Note that we only call it if APR_USE_SYSVSEM_SERIALIZE is defined,
but the code itself is a noop unless APR_HAS_SYSVSEM_SERIALIZE is defined
(defined means 1). Good catch.

-- 
===
   Jim Jagielski   [|]   [EMAIL PROTECTED]   [|]   http://www.jaguNET.com/
  "A society that will trade a little liberty for a little order
 will lose both and deserve neither" - T.Jefferson

Re: [patch] two small problems in ssl_engine_mutex.c

2003-03-30 Thread Brian Pane 
+1

Brian

On Sun, 2003-03-30 at 22:53, William A. Rowe, Jr. wrote:
> One, win32 won't compile (nor any platform missing chown).  In this
> case we didn't need it and have a good macro to look at.
> 
> This raised another bug in the next line.  We assumed because we
> default to SYSV mutexes we should do that magic.  I believe this is
> wrong, and we should be looking for that sort of lock explicitly.
> 
> Please review.
> 
> Bill
>

Re: [patch] two small problems in ssl_engine_mutex.c

2003-03-30 Thread Justin Erenkrantz 
--On Monday, March 31, 2003 12:53 AM -0600 "William A. Rowe, Jr." 
<[EMAIL PROTECTED]> wrote:

One, win32 won't compile (nor any platform missing chown).  In this
case we didn't need it and have a good macro to look at.
Yup.  Bad.  It'd never get executed on them, but how is the compiler to know?

This raised another bug in the next line.  We assumed because we
default to SYSV mutexes we should do that magic.  I believe this is
wrong, and we should be looking for that sort of lock explicitly.
Yup.  But, as a stylistic nit, I'd prefer following what prefork.c:977 has 
instead.  So:

+ #if APR_HAS_SYSVSEM_SERIALIZE
+if (((mc->nMutexMech == APR_LOCK_DEFAULT) && APR_USE_SYSVSEM_SERIALIZE)
+|| (mc->nMutexMech == APR_LOCK_SYSVSEM)) {
would be:

#if APR_HAS_SYSVSEM_SERIALIZE
#if APR_USE_SYSVSEM_SERIALIZE
   if (mc->nMutexMech == APR_LOCK_DEFAULT ||
   mc->nMutexMech == APR_LOCK_SYSVSEM) {
#else
   if (mc->nMutexMech == APR_LOCK_SYSVSEM) {
#endif
...
Take this for whatever it's worth.  Looks good.  +1.  -- justin

[patch] two small problems in ssl_engine_mutex.c

2003-03-30 Thread William A. Rowe, Jr. 
One, win32 won't compile (nor any platform missing chown).  In this
case we didn't need it and have a good macro to look at.

This raised another bug in the next line.  We assumed because we
default to SYSV mutexes we should do that magic.  I believe this is
wrong, and we should be looking for that sort of lock explicitly.

Please review.

BillIndex: modules/ssl/ssl_engine_mutex.c
===
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_mutex.c,v
retrieving revision 1.17.2.4
diff -u -r1.17.2.4 ssl_engine_mutex.c
--- modules/ssl/ssl_engine_mutex.c  30 Mar 2003 23:17:22 -  1.17.2.4
+++ modules/ssl/ssl_engine_mutex.c  31 Mar 2003 06:50:40 -
@@ -84,16 +84,21 @@
  "Cannot create SSLMutex");
 return FALSE;
 }
+#if APR_HAS_FLOCK_SERIALIZE
 if (mc->szMutexFile && mc->ChownMutexFile == TRUE)
 chown(mc->szMutexFile, unixd_config.user_id, -1);
+#endif
 
-#if APR_USE_SYSVSEM_SERIALIZE
-rv = unixd_set_global_mutex_perms(mc->pMutex);
-if (rv != APR_SUCCESS) {
-ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
- "Could not set permissions on ssl_mutex; check User "
- "and Group directives");
-return FALSE;
+#if APR_HAS_SYSVSEM_SERIALIZE
+if (((mc->nMutexMech == APR_LOCK_DEFAULT) && APR_USE_SYSVSEM_SERIALIZE)
+|| (mc->nMutexMech == APR_LOCK_SYSVSEM)) {
+rv = unixd_set_global_mutex_perms(mc->pMutex);
+if (rv != APR_SUCCESS) {
+ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
+ "Could not set permissions on ssl_mutex; check User "
+ "and Group directives");
+return FALSE;
+}
 }
 #endif
 return TRUE;