Joe Orton
Mon, 21 Jul 2008 03:47:52 -0700
On Sat, Jul 19, 2008 at 07:13:08AM +0200, Mladen Turk wrote:
...
> Index: locks/unix/proc_mutex.c
> ===================================================================
> --- locks/unix/proc_mutex.c (revision 677948)
> +++ locks/unix/proc_mutex.c (working copy)
> @@ -915,6 +915,40 @@
> return NULL;
> }
>
> +APR_DECLARE(apr_status_t) apr_proc_mutex_set_perms(apr_proc_mutex_t *mutex,
> + apr_fileperms_t perms,
> + apr_uid_t *uid,
> + apr_gid_t *gid)
uid/gid should be passed directly rather than by reference, I think.
> +{
> +
> + if (!geteuid()) {
That seems out-of-place. Either it should be an API guarantee that this
call is a no-op if the euid is not 0, or that should be left to the
caller. I think the latter makes more sense.
> +#if APR_HAS_SYSVSEM_SERIALIZE
> + if (mutex->meth == &mutex_sysv_methods) {
To be consistent with the rest of this code, the implementation should
use add a new function in the _lock_methods_t vtable and add new
method-specific functions.
...
> +#endif
> +#if APR_HAS_FLOCK_SERIALIZE
> + if (mutex->meth == &mutex_flock_methods) {
> + if (mutex->fname) {
Is this necessary? AFAICT ->fname should always be non-NULL.
> + if (chown(mutex->fname, *uid,
> + -1 /* no gid change */) < 0) {
Where available, using fchown() on the fd instead of chown() would be
more efficient/safer.
...
> /**
> + * Set mutex perimissions.
spello for "permissions".
> + * @param mutex the mutex to set.
> + * @param perms Access permissions for the mutex. Mimics Unix access rights.
What does "Mimics Unix access rights" mean? How will this mimic Unix on
non-Unix platforms? It seems superfluous, I'd remove the phrase.
Regards,
joe