Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Date: Thu, 23 May 2002 16:16:32 -0700
From: Justin Erenkrantz <jerenkrantz@apache.org>
To: dev@apr.apache.org
Cc: dberti777@yahoo.com
Subject: Re: QNX 6.1a mod/peer review
Message-ID: <20020523161632.H27903@apache.org>
Mail-Followup-To: Justin Erenkrantz <jerenkrantz@apache.org>,
	dev@apr.apache.org, dberti777@yahoo.com
References: <1022174480.75812.ezmlm@httpd.apache.org>
 <20020523172808.29935.qmail@web20009.mail.yahoo.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5i
In-Reply-To: <20020523172808.29935.qmail@web20009.mail.yahoo.com>;
 from dberti777@yahoo.com on Thu, May 23, 2002 at 10:28:08AM -0700

[ Moving to dev@apr which is the right list for this. ]

Comments inline.

On Thu, May 23, 2002 at 10:28:08AM -0700, Davide Berti wrote:
> --- httpd-2.0.36/srclib/apr/locks/unix/proc_mutex.c
> Mon Apr  8 23:56:56 2002
> +++ ../httpd-2.0.36/srclib/apr/locks/unix/proc_mutex.c
> Wed May  8 16:04:51 2002
> @@ -318,7 +318,9 @@
>          if (munmap((caddr_t)mutex->pthread_interproc,
> sizeof(pthread_mutex_t))){
>              return errno;
>          }
> -    }
> +    	if(shm_unlink("/datapoints")) // DB
> +            return errno;
> +	}

Stylistic concerns:
Lose the // DB comments and the tabs.  We only use spaces (4 space
indentions).  You should read our style guide:

http://httpd.apache.org/dev/styleguide.html

Does QNX not have /dev/zero?  What was the problem you were seeing?

What is /datapoints?  Obviously, your patch breaks other platforms.
If you wish us to integrate it into our sources, you need to make
it so that it doesn't break other platforms.  Since it sounds
like /dev/zero doesn't exist, you need a shared data source -
is /datapoints a QNX convention or something you came up with?
If this isn't a "special file", you'll have a few problems:

1) How will other users (non-root) be able to access this file?
(ISTR that QNX doesn't have users, so that may be why.)

2) If there is a "dead" process that died due to a segfault,
this file won't be removed, so you'll have to remove this file
yourself.  (This is why SysV mutexes are so problematic.)

> @@ -329,11 +331,15 @@
>      int fd;
>      pthread_mutexattr_t mattr;
>  
> -    fd = open("/dev/zero", O_RDWR);
> -    if (fd < 0) {
> -        return errno;
> -    }
> +    fd=shm_open("/datapoints",O_RDWR|O_CREAT,0777);
> //DB
> +	if (fd < 0) 
> +		return errno;	
>  
> +	if(ftruncate(fd,sizeof(pthread_mutex_t))==-1)  //DB
> +		return errno;	
> +
> +
> +

Lose the extra blank lines.  You also removed the {} on one-line
if statements which is our style.

Any particular reason why the fd must be truncated?  We only use
/dev/zero because it's a cheap place to get file-backed storage - we
don't rely upon the fact that it is zeroed out.  So, you should be
able to get away without this.

> @@ -363,10 +369,11 @@
>                                                
> PTHREAD_MUTEX_ROBUST_NP))) {
>  #ifdef PTHREAD_SETS_ERRNO
>          rv = errno;
> -#endif
> +#endif // DB
>          proc_mutex_proc_pthread_cleanup(new_mutex);
>          return rv;
>      }
> +#endif
>      if ((rv = pthread_mutexattr_setprotocol(&mattr,
> PTHREAD_PRIO_INHERIT))) {
>  #ifdef PTHREAD_SETS_ERRNO
>          rv = errno;

You added an #endif - why?

> @@ -374,9 +381,15 @@
>          proc_mutex_proc_pthread_cleanup(new_mutex);
>          return rv;
>      }
> +    if ((rv =
> pthread_mutex_destroy(new_mutex->pthread_interproc)))
> { // DB
> +#ifdef PTHREAD_SETS_ERRNO
> +        rv = errno;
>  #endif
> +        proc_mutex_proc_pthread_cleanup(new_mutex);
> +        return rv;
> +    }
>  
> -    if ((rv =
> pthread_mutex_init(new_mutex->pthread_interproc,
> &mattr))) {
> +	if ((rv =
> pthread_mutex_init(new_mutex->pthread_interproc,
> &mattr))) {
>  #ifdef PTHREAD_SETS_ERRNO
>          rv = errno;
>  #endif

Why call destroy before calling init?  (Lose your tabs->space
conversion, too.)

All in all, this might not be a bad road to proceed down so that
on platforms without /dev/zero, we could use pthread cross-process
mutexes, but I doubt it'll be easy.

I know our autoconf test won't recognize pthread proc mutex in
this case since the test checks for /dev/zero.  So, you'll have
to modify that as weel to get this to be included.  -- justin