apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apr/locks/win32 locks.c
Date Tue, 05 Jun 2001 17:47:32 GMT
Oh, geez. 

This is really ugly. It is creating dual paths all over. And the lock
structure now has a couple variants that need to be checked/tested.

Why can't we have a new pool type? The pool structure is now private (I made
it private about a month ago). We can change bits under the covers pretty
easily.

I'm not talking about replacing the pools, but just having a small variant.

And why is apr_lock_sms_create() declared in the private header files rather
than the public apr_locks.h ?

I *really* don't like this patch. Especially those macros in acconfig.h. I'm
tempted to say back it out and let's do the pool variant instead. Wanted to
hear some ideas/discussion before asking for a back-out, though.

Cheers,
-g

On Tue, Jun 05, 2001 at 08:15:40AM -0000, dreid@apache.org wrote:
> dreid       01/06/05 01:15:40
> 
>   Modified:    .        acconfig.h
>                include  apr.h.in apr_lock.h apr_sms.h
>                include/arch/beos locks.h
>                include/arch/os2 locks.h
>                include/arch/unix locks.h
>                include/arch/win32 locks.h
>                locks/unix crossproc.c locks.c
>                locks/win32 locks.c
>   Log:
>   OK, this basically adds a function that allows us to create
>   apr_lock_t's using an apr_sms_t for the memory.  It's a very small first
>   step, and at present is only intended to be used internally in APR,
>   hence the position of the function definitions in the locks.h file.
>   
>   Given that we don't want to duplicate code where we don't have to, I've
>   added some macros that allow us to do memory allocations regardless of
>   whether we have a pool or sms.  This also highlighted that we haven't
>   yet managed to change our member names from cntxt to pool everywhere!
>   
>   The surprise comes in just how far reaching the apr_pool_t goes.
>   
>   As apr_lock.h uses ap_sms_t and apr_sms.h uses apr_lock_t I've moved
>   the tyepdef's into apr.h.in, but this may be bogus...
>   
>   Hopefully this will allow us to get locking into sms and so start
>   moving forward again, and at the same time it starts to throw up
>   the problems for changing our memory system throughout APR.
>   
>   Revision  Changes    Path
>   1.43      +42 -0     apr/acconfig.h
>   
>   Index: acconfig.h
>   ===================================================================
>   RCS file: /home/cvs/apr/acconfig.h,v
>   retrieving revision 1.42
>   retrieving revision 1.43
>   diff -u -r1.42 -r1.43
>   --- acconfig.h	2001/04/19 07:18:38	1.42
>   +++ acconfig.h	2001/06/05 08:15:01	1.43
>   @@ -67,4 +67,46 @@
>    #define apr_sigwait(a,b) sigwait((a),(b))
>    #endif
>    
>   +/* Macros to deal with using either a pool or an sms
>   + * to do memory stuff...
>   + */
>   +#define APR_REGISTER_CLEANUP(struct, data, func, scope) \
>   +    if (struct->cntxt) { \
>   +        apr_pool_cleanup_register(struct->cntxt, data, func, scope); \
>   +    } else { \
>   +        apr_sms_cleanup_register(struct->mem_sys, APR_CHILD_CLEANUP, \
>   +                                 data, func); \
>   +    }
>   +
>   +#define APR_REMOVE_CLEANUP(struct, data, func) \
>   +    if (struct->cntxt) { \
>   +        apr_pool_cleanup_kill(struct->cntxt, data, func); \
>   +    } else { \
>   +        apr_sms_cleanup_unregister(struct->mem_sys, APR_CHILD_CLEANUP, \
>   +                                   data, func); \
>   +    }
>   +
>   +#define APR_MEM_PSTRDUP(struct, ptr, str) \
>   +    if (struct->cntxt) { \
>   +        ptr = apr_pstrdup(struct->cntxt, str); \
>   +    } else { \
>   +        size_t len = strlen(str) + 1; \
>   +        ptr = (char*) apr_sms_calloc(struct->mem_sys, len); \
>   +        memcpy(ptr, str, len); \
>   +    }
>   +
>   +#define APR_MEM_MALLOC(ptr, struct, type) \
>   +    if (struct->cntxt) { \
>   +        ptr = (type *)apr_palloc(struct->cntxt, sizeof(type)); \
>   +    } else { \
>   +        ptr = (type *)apr_sms_malloc(struct->mem_sys, sizeof(type)); \
>   +    }
>   +
>   +#define APR_MEM_CALLOC(ptr, struct, type) \
>   +    if (struct->cntxt) { \
>   +        ptr = (type *)apr_pcalloc(struct->cntxt, sizeof(type)); \
>   +    } else { \
>   +        ptr = (type *)apr_sms_calloc(struct->mem_sys, sizeof(type)); \
>   +    }
>   +
>    #endif /* APR_PRIVATE_H */
>   
>   
>   
>   1.81      +2 -0      apr/include/apr.h.in
>   
>   Index: apr.h.in
>   ===================================================================
>   RCS file: /home/cvs/apr/include/apr.h.in,v
>   retrieving revision 1.80
>   retrieving revision 1.81
>   diff -u -r1.80 -r1.81
>   --- apr.h.in	2001/05/10 18:09:26	1.80
>   +++ apr.h.in	2001/06/05 08:15:05	1.81
>   @@ -174,6 +174,8 @@
>    typedef  @off_t_value@           apr_off_t;
>    typedef  @socklen_t_value@       apr_socklen_t;
>    
>   +typedef struct apr_lock_t        apr_lock_t;
>   +typedef struct apr_sms_t         apr_sms_t;
>    
>    /* Mechanisms to properly type numeric literals */
>    @int64_literal@
>   
>   
>   
>   1.25      +1 -2      apr/include/apr_lock.h
>   
>   Index: apr_lock.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/apr_lock.h,v
>   retrieving revision 1.24
>   retrieving revision 1.25
>   diff -u -r1.24 -r1.25
>   --- apr_lock.h	2001/05/31 03:29:53	1.24
>   +++ apr_lock.h	2001/06/05 08:15:07	1.25
>   @@ -58,6 +58,7 @@
>    #include "apr.h"
>    #include "apr_pools.h"
>    #include "apr_errno.h"
>   +#include "apr_sms.h"
>    
>    #ifdef __cplusplus
>    extern "C" {
>   @@ -72,8 +73,6 @@
>    typedef enum {APR_MUTEX, APR_READWRITE} apr_locktype_e;
>    
>    typedef enum {APR_READER, APR_WRITER} apr_readerwriter_e;
>   -
>   -typedef struct apr_lock_t           apr_lock_t;
>    
>    /*   Function definitions */
>    
>   
>   
>   
>   1.9       +1 -2      apr/include/apr_sms.h
>   
>   Index: apr_sms.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/apr_sms.h,v
>   retrieving revision 1.8
>   retrieving revision 1.9
>   diff -u -r1.8 -r1.9
>   --- apr_sms.h	2001/06/04 23:09:32	1.8
>   +++ apr_sms.h	2001/06/05 08:15:08	1.9
>   @@ -79,8 +79,6 @@
>     * @package APR memory system
>     */
>    
>   -typedef struct apr_sms_t apr_sms_t;
>   -
>    struct apr_sms_cleanup;
>    
>    /**
>   @@ -94,6 +92,7 @@
>      apr_sms_t **ref_mem_sys;
>      apr_sms_t  *accounting_mem_sys;
>      const char *identity; /* a string identifying the module */
>   +  apr_lock_t *lock;
>    
>      struct apr_sms_cleanup *cleanups;
>    
>   
>   
>   
>   1.12      +5 -0      apr/include/arch/beos/locks.h
>   
>   Index: locks.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/arch/beos/locks.h,v
>   retrieving revision 1.11
>   retrieving revision 1.12
>   diff -u -r1.11 -r1.12
>   --- locks.h	2001/02/16 04:15:49	1.11
>   +++ locks.h	2001/06/05 08:15:14	1.12
>   @@ -60,9 +60,11 @@
>    #include "apr_file_io.h"
>    #include "apr_general.h"
>    #include "apr_lib.h"
>   +#include "apr_sms.h"
>    
>    struct apr_lock_t {
>        apr_pool_t *cntxt;
>   +    apr_sms_t *mem_sys;
>        apr_locktype_e type;
>        apr_lockscope_e scope;
>        /* Inter proc */
>   @@ -86,6 +88,9 @@
>    apr_status_t child_init_lock(struct apr_lock_t **lock, apr_pool_t *cont, 
>                                const char *fname);
>    
>   +apr_status_t apr_lock_sms_create(apr_lock_t **lock, apr_locktype_e type,
>   +                                 apr_lockscope_e scope, const_char *fname,
>   +                                 apr_sms_t *mem_sys);
>    
>    #endif  /* LOCKS_H */
>    
>   
>   
>   
>   1.12      +6 -0      apr/include/arch/os2/locks.h
>   
>   Index: locks.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/arch/os2/locks.h,v
>   retrieving revision 1.11
>   retrieving revision 1.12
>   diff -u -r1.11 -r1.12
>   --- locks.h	2001/03/19 12:43:24	1.11
>   +++ locks.h	2001/06/05 08:15:18	1.12
>   @@ -57,9 +57,11 @@
>    
>    #include "apr_lock.h"
>    #include "apr_file_io.h"
>   +#include "apr_sms.h"
>    
>    struct apr_lock_t {
>        apr_pool_t *cntxt;
>   +    apr_sms_t *mem_sys;
>        apr_locktype_e type;
>        apr_lockscope_e scope;
>        char *fname;
>   @@ -68,6 +70,10 @@
>        int lock_count;
>        TIB *tib;
>    };
>   +
>   +apr_status_t apr_lock_sms_create(apr_lock_t **lock, apr_locktype_e type,
>   +                                 apr_lockscope_e scope, const char *fname,
>   +                                 apr_sms_t *mem_sys);
>    
>    #endif  /* LOCKS_H */
>    
>   
>   
>   
>   1.28      +6 -0      apr/include/arch/unix/locks.h
>   
>   Index: locks.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/arch/unix/locks.h,v
>   retrieving revision 1.27
>   retrieving revision 1.28
>   diff -u -r1.27 -r1.28
>   --- locks.h	2001/05/31 03:29:54	1.27
>   +++ locks.h	2001/06/05 08:15:22	1.28
>   @@ -60,6 +60,7 @@
>    #include "apr_general.h"
>    #include "apr_lib.h"
>    #include "apr_lock.h"
>   +#include "apr_sms.h"
>    
>    /* System headers required by Locks library */
>    #if APR_HAVE_SYS_TYPES_H
>   @@ -109,6 +110,7 @@
>    
>    struct apr_lock_t {
>        apr_pool_t *cntxt;
>   +    apr_sms_t *mem_sys;
>        apr_locktype_e type;
>        apr_lockscope_e scope;
>        int curr_locked;
>   @@ -154,6 +156,10 @@
>    
>    apr_status_t apr_unix_child_init_lock(struct apr_lock_t **lock, 
>                                          apr_pool_t *cont, const char *fname);
>   +
>   +apr_status_t apr_lock_sms_create(apr_lock_t **lock, apr_locktype_e type,
>   +                                 apr_lockscope_e scope, const char *fname,
>   +                                 apr_sms_t *mem_sys);
>    
>    #endif  /* LOCKS_H */
>    
>   
>   
>   
>   1.9       +6 -0      apr/include/arch/win32/locks.h
>   
>   Index: locks.h
>   ===================================================================
>   RCS file: /home/cvs/apr/include/arch/win32/locks.h,v
>   retrieving revision 1.8
>   retrieving revision 1.9
>   diff -u -r1.8 -r1.9
>   --- locks.h	2001/02/16 04:15:52	1.8
>   +++ locks.h	2001/06/05 08:15:26	1.9
>   @@ -56,15 +56,21 @@
>    #define LOCKS_H
>    
>    #include "apr_lock.h"
>   +#include "apr_sms.h"
>    
>    struct apr_lock_t {
>        apr_pool_t *cntxt;
>   +    apr_sms_t *mem_sys;
>        apr_locktype_e type;
>        apr_lockscope_e scope;
>        HANDLE mutex;
>        CRITICAL_SECTION section;
>        char *fname;
>    };
>   +
>   +apr_status_t apr_lock_sms_create(apr_lock_t **lock, apr_locktype_e type,
>   +                                 apr_lockscope_e scope, const char *fname,
>   +                                 apr_sms_t *mem_sys);
>    
>    #endif  /* LOCKS_H */
>    
>   
>   
>   
>   1.43      +11 -10    apr/locks/unix/crossproc.c
>   
>   Index: crossproc.c
>   ===================================================================
>   RCS file: /home/cvs/apr/locks/unix/crossproc.c,v
>   retrieving revision 1.42
>   retrieving revision 1.43
>   diff -u -r1.42 -r1.43
>   --- crossproc.c	2001/02/23 20:29:07	1.42
>   +++ crossproc.c	2001/06/05 08:15:30	1.43
>   @@ -100,7 +100,7 @@
>            return errno;
>        }
>        new->curr_locked = 0;
>   -    apr_pool_cleanup_register(new->cntxt, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>   +    APR_REGISTER_CLEANUP(new, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>        return APR_SUCCESS;
>    }
>    
>   @@ -137,7 +137,7 @@
>        apr_status_t stat;
>    
>        if ((stat = lock_cleanup(lock)) == APR_SUCCESS) {
>   -        apr_pool_cleanup_kill(lock->cntxt, lock, lock_cleanup);
>   +        APR_REMOVE_CLEANUP(lock, lock, lock_cleanup);
>            return APR_SUCCESS;
>        }
>        return stat;
>   @@ -223,7 +223,7 @@
>        }
>    
>        new->curr_locked = 0;
>   -    apr_pool_cleanup_register(new->cntxt, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>   +    APR_REGISTER_CLEANUP(new, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>        return APR_SUCCESS;
>    }
>    
>   @@ -259,7 +259,7 @@
>    {
>        apr_status_t stat;
>        if ((stat = lock_cleanup(lock)) == APR_SUCCESS) {
>   -        apr_pool_cleanup_kill(lock->cntxt, lock, lock_cleanup);
>   +        APR_REMOVE_CLEANUP(lock, lock, lock_cleanup);
>            return APR_SUCCESS;
>        }
>        return stat;
>   @@ -305,7 +305,7 @@
>            new->interproc = open(new->fname, O_CREAT | O_WRONLY | O_EXCL, 0644);
>        }
>        else {
>   -        new->fname = apr_pstrdup(new->cntxt, "/tmp/aprXXXXXX"); 
>   +        APR_MEM_PSTRDUP(new, new->fname, "/tmp/aprXXXXXX")
>            new->interproc = apr_mkstemp(new->fname);
>        }
>    
>   @@ -316,7 +316,7 @@
>    
>        new->curr_locked=0;
>        unlink(new->fname);
>   -    apr_pool_cleanup_register(new->cntxt, new, lock_cleanup, apr_pool_cleanup_null);
>   +    APR_REGISTER_CLEANUP(new, new, lock_cleanup, apr_pool_cleanup_null);
>        return APR_SUCCESS; 
>    }
>    
>   @@ -352,7 +352,7 @@
>    {
>        apr_status_t stat;
>        if ((stat = lock_cleanup(lock)) == APR_SUCCESS) {
>   -        apr_pool_cleanup_kill(lock->cntxt, lock, lock_cleanup);
>   +        APR_REMOVE_CLEANUP(lock, lock, lock_cleanup);
>            return APR_SUCCESS;
>        }
>        return stat;
>   @@ -364,6 +364,7 @@
>        return APR_SUCCESS;
>    }
>    
>   +
>    #elif (APR_USE_FLOCK_SERIALIZE)
>    
>    void apr_unix_setup_lock(void)
>   @@ -387,7 +388,7 @@
>            new->interproc = open(new->fname, O_CREAT | O_WRONLY | O_EXCL, 0600);
>        }
>        else {
>   -        new->fname = apr_pstrdup(new->cntxt, "/tmp/aprXXXXXX"); 
>   +        APR_MEM_PSTRDUP(new, new->fname, "/tmp/aprXXXXXX")
>            new->interproc = apr_mkstemp(new->fname);
>        }
>    
>   @@ -396,7 +397,7 @@
>            return errno;
>        }
>        new->curr_locked = 0;
>   -    apr_pool_cleanup_register(new->cntxt, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>   +    APR_REGISTER_CLEANUP(new, (void *)new, lock_cleanup, apr_pool_cleanup_null);
>        return APR_SUCCESS;
>    }
>    
>   @@ -432,7 +433,7 @@
>    {
>        apr_status_t stat;
>        if ((stat = lock_cleanup(lock)) == APR_SUCCESS) {
>   -        apr_pool_cleanup_kill(lock->cntxt, lock, lock_cleanup);
>   +        APR_REMOVE_CLEANUP(lock, lock, lock_cleanup);
>            return APR_SUCCESS;
>        }
>        return stat;
>   
>   
>   
>   1.48      +45 -16    apr/locks/unix/locks.c
>   
>   Index: locks.c
>   ===================================================================
>   RCS file: /home/cvs/apr/locks/unix/locks.c,v
>   retrieving revision 1.47
>   retrieving revision 1.48
>   diff -u -r1.47 -r1.48
>   --- locks.c	2001/05/31 03:29:59	1.47
>   +++ locks.c	2001/06/05 08:15:32	1.48
>   @@ -56,46 +56,38 @@
>    #include "apr_strings.h"
>    #include "apr_portable.h"
>    
>   -apr_status_t apr_lock_create(apr_lock_t **lock, apr_locktype_e type, 
>   -                           apr_lockscope_e scope, const char *fname, 
>   -                           apr_pool_t *cont)
>   +static apr_status_t create_lock(apr_lock_t *new, const char *fname)
>    {
>   -    apr_lock_t *new;
>        apr_status_t stat;
>    
>   -    new = (apr_lock_t *)apr_pcalloc(cont, sizeof(apr_lock_t));
>   -
>   -    new->cntxt = cont;
>   -    new->type  = type;
>   -    new->scope = scope;
>        switch (new->type)
>        {
>        case APR_MUTEX:
>    #if (APR_USE_FCNTL_SERIALIZE) || (APR_USE_FLOCK_SERIALIZE)
>        /* file-based serialization primitives */
>   -    if (scope != APR_INTRAPROCESS) {
>   +    if (new->scope != APR_INTRAPROCESS) {
>            if (fname != NULL) {
>   -            new->fname = apr_pstrdup(cont, fname);
>   +            APR_MEM_PSTRDUP(new, new->fname, fname);
>            }
>        }
>    #endif
>    
>    #if APR_PROCESS_LOCK_IS_GLOBAL /* don't need intra lock for APR_LOCKALL */
>   -    if (scope == APR_INTRAPROCESS) {
>   +    if (new->scope == APR_INTRAPROCESS) {
>    #else
>   -    if (scope != APR_CROSS_PROCESS) {
>   +    if (new->scope != APR_CROSS_PROCESS) {
>    #endif
>    #if APR_HAS_THREADS
>            if ((stat = apr_unix_create_intra_lock(new)) != APR_SUCCESS) {
>                return stat;
>            }
>    #else
>   -        if (scope != APR_LOCKALL) {
>   +        if (new->scope != APR_LOCKALL) {
>                return APR_ENOTIMPL;
>            }
>    #endif
>        }
>   -    if (scope != APR_INTRAPROCESS) {
>   +    if (new->scope != APR_INTRAPROCESS) {
>            if ((stat = apr_unix_create_inter_lock(new)) != APR_SUCCESS) {
>                return stat;
>            }
>   @@ -103,7 +95,7 @@
>        break;
>        case APR_READWRITE:
>    #ifdef HAVE_PTHREAD_RWLOCK_INIT
>   -    if (scope != APR_INTRAPROCESS)
>   +    if (new->scope != APR_INTRAPROCESS)
>            return APR_ENOTIMPL;
>        pthread_rwlock_init(&new->rwlock, NULL);
>        break;
>   @@ -111,6 +103,43 @@
>        return APR_ENOTIMPL;
>    #endif
>        }
>   +    return APR_SUCCESS;
>   +}
>   +
>   +apr_status_t apr_lock_create(apr_lock_t **lock, apr_locktype_e type, 
>   +                           apr_lockscope_e scope, const char *fname, 
>   +                           apr_pool_t *cont)
>   +{
>   +    apr_lock_t *new;
>   +    apr_status_t stat;
>   +
>   +    new = (apr_lock_t *)apr_pcalloc(cont, sizeof(apr_lock_t));
>   +
>   +    new->cntxt = cont;
>   +    new->type  = type;
>   +    new->scope = scope;
>   +    if ((stat = create_lock(new, fname)) != APR_SUCCESS)
>   +        return APR_SUCCESS;
>   +
>   +    *lock = new;
>   +    return APR_SUCCESS;
>   +}
>   +
>   +apr_status_t apr_lock_sms_create(apr_lock_t **lock, apr_locktype_e type,
>   +                                 apr_lockscope_e scope, const char *fname,
>   +                                 apr_sms_t *mem_sys)
>   +{
>   +    apr_lock_t *new;
>   +    apr_status_t stat;
>   +    
>   +    new = (apr_lock_t *)apr_sms_calloc(mem_sys, sizeof(apr_lock_t));
>   +
>   +    new->mem_sys = mem_sys;
>   +    new->cntxt = NULL;
>   +    new->type = type;
>   +    new->scope = scope;
>   +    if ((stat = create_lock(new, fname)) != APR_SUCCESS)
>   +        return stat;
>    
>        *lock = new;
>        return APR_SUCCESS;
>   
>   
>   
>   1.39      +39 -0     apr/locks/win32/locks.c
>   
>   Index: locks.c
>   ===================================================================
>   RCS file: /home/cvs/apr/locks/win32/locks.c,v
>   retrieving revision 1.38
>   retrieving revision 1.39
>   diff -u -r1.38 -r1.39
>   --- locks.c	2001/06/01 15:05:18	1.38
>   +++ locks.c	2001/06/05 08:15:37	1.39
>   @@ -98,6 +98,45 @@
>        return APR_SUCCESS;
>    }
>    
>   +APR_DECLARE(apr_status_t) apr_lock_sms_create(apr_lock_t **lock,
>   +                                              apr_locktype_e type,
>   +                                              apr_lockscope_e scope,
>   +                                              const char *fname,
>   +                                              apr_sms_t *mem_sys)
>   +{
>   +    apr_lock_t *newlock;
>   +    SECURITY_ATTRIBUTES sec;
>   +
>   +    if (type == APR_READWRITE)
>   +        return APR_ENOTIMPL;
>   +
>   +    newlock = (apr_lock_t *)apr_sms_malloc(mem_sys, sizeof(apr_lock_t));
>   +
>   +    newlock->cntxt = NULL;
>   +    newlock->mem_sys = mem_sys;
>   +
>   +    APR_MEM_PSTRDUP(newlock, newlock->fname, fname);
>   +    newlock->type = type;
>   +    newlock->scope = scope;
>   +    sec.nLength = sizeof(SECURITY_ATTRIBUTES);
>   +    sec.lpSecurityDescriptor = NULL;
>   +
>   +    if (scope == APR_CROSS_PROCESS || scope == APR_LOCKALL) {
>   +        sec.bInheritHandle = TRUE;
>   +    }
>   +    else {
>   +        sec.bInheritHandle = FALSE;
>   +    }
>   +
>   +    if (scope == APR_INTRAPROCESS) {
>   +        InitializeCriticalSection(&newlock->section);
>   +    } else {
>   +        newlock->mutex = CreateMutex(&sec, FALSE, fname);
>   +    }
>   +    *lock = newlock;
>   +    return APR_SUCCESS;
>   +}
>   +
>    APR_DECLARE(apr_status_t) apr_lock_child_init(apr_lock_t **lock, 
>                                                  const char *fname, 
>                                                  apr_pool_t *cont)
>   
>   
>   

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message