apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1)" <madhusudan_mathiha...@hp.com>
Subject RE: [PATCH] shmem.c - again
Date Fri, 14 Sep 2001 19:03:50 GMT
Thanks for your comments. I'll include them and try to send out another
patch.. As regards your comment regarding "works only with shmget" - you're
right..I sent out the patch to mainly get your comments regarding the memory
mgmt that can be done. I'll certainly look into the possibilities of using
mmap, as soon as the proposed logic is accepted.

-Madhu

-----Original Message-----
From: Justin Erenkrantz [mailto:jerenkrantz@ebuilt.com]
Sent: Friday, September 14, 2001 10:00 AM
To: MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1)
Cc: dev@httpd.apache.org; 'dev@apr.apache.org'
Subject: Re: [PATCH] shmem.c - again


Comments inline.

> Index: shmem.c
> ===================================================================
> RCS file: /home/cvspublic/apr/shmem/unix/shmem.c,v
> retrieving revision 1.33
> diff -u -r1.33 shmem.c
> --- shmem.c	2001/08/30 17:11:04	1.33
> +++ shmem.c	2001/09/12 21:07:29
> @@ -89,182 +89,593 @@
>  #include <kernel/OS.h>
>  #endif
>  
> +#define MIN_BLK_SIZE 200

Where did this number come from?  It should at least be a power of two.

> +
>
+/**************************************************************************
*
> +The Shared Memory - Management Architecture :
> +
> +List Memory (Shared Memory - 1)
> +     ____________________________________________
> +     | Memory Offsets for the List and          |
> +     |        for the Shared Memory             |
> +     |------------------------------------------|
> +L1   | MemChunk(MC1) |Next| MemChunk(MC2) |Next |
> +     |------------------------------------------|
> +     |------------------------------------------|
> +L2   | MemChunk(MC3) |Next| MemChunk(MC4) |Next |
> +     |------------------------------------------|
> +   ______|      .  .  .  .           . .        .
> +   | .          .  .  .  .           . .        .
> +   | .          .  .  .  .           . .        .
> +   | |------------------------------------------|
> +Ln | | MemChunk(MCm) |Next|        ......       |
> +   | |------------------------------------------|
> +   |   |
> +   |   |________________________________________
> +   V                                            | (offset from shm_base)
> +Chunk Memory (Shared Memory - 2)                V
> +-------------------------------------------------------------------------
> +|         |          |                 |     |       |                  |
> +|   C1    |    C2    |       C3        | C4  |  C5   |....              |
> +|         |          |                 |     |       |                  |
> +-------------------------------------------------------------------------
> +
>
+***************************************************************************
/
> +
> +
> +typedef struct memoffsets_t {
> +    apr_off_t      l_used;     /* Starting of the Used list elements */
> +    apr_off_t      l_free;     /* Starting of the Freed list elements */
> +    apr_off_t      l_usedlast; /* Last element of the Used list */
> +    apr_off_t      l_freelast; /* Last element of the Freed list */
> +    apr_off_t      c_used;     /* Begining of the used chunk list */
> +    apr_off_t      c_free;     /* Begining of the freed chunk list */
> +    apr_off_t      shm_offset; /* The current offset of the shared memory
*/
> +    apr_off_t      shm_length; /* Total length of shared memory available
*/
> +} memoffsets_t;
> +
> +typedef struct memchunk_t {
> +    apr_off_t      offset;     /* Offset of the chunk - from m->shm_base
*/
> +    apr_size_t     size;       /* Size of the chunk */
> +    apr_off_t      next;       /* Next chunk - from m->list_base) */
> +    apr_off_t      prev;       /* Previous chunk - from m->list_base) */
> +} memchunk_t;
> +
> +typedef struct memlist_t {
> +    struct memchunk_t  chunk;
> +    apr_off_t          next;
> +} memlist_t;
> +
>  struct shmem_t {
> -    void *mem;
> -    void *curmem;
> -    apr_size_t length;
> -    apr_lock_t *lock;
> -    char *filename;
> +    apr_pool_t      *p;
> +    void            *shm_base;    /* Starting address of the shared
memory */
> +    memoffsets_t    *offsets;     /* Begining of the set of offsets */
> +    memlist_t       *list_base;   /* Begining of the list elements */
> +
> +    apr_lock_t      *lock;
> +    char            *filename;
>  #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM ||
APR_USE_SHMEM_MMAP_ZERO
> -    apr_file_t *file; 
> -#elif APR_USE_SHMEM_MMAP_ANON
> -    /* Nothing else. */
> +    apr_file_t      *file; 
>  #elif APR_USE_SHMEM_SHMGET
> -    apr_os_file_t file;
> +    apr_os_file_t    file;
> +    apr_os_file_t    listfd;
>  #elif APR_USE_SHMEM_BEOS
> -    area_id areaid; 
> +    area_id          areaid; 
>  #endif

As a matter of style, I don't like the extra spaces in between the type 
and the variable name.  One space please.  The formatting changes make
it hard to see what it is you have changed.  Please lose them.  I
think you have added some extra variables that are no longer needed.
(I only have email access while I am stranded, so I can't apply your
patch.)

> +#define SHM_ADDR(offset)    ((offset < 0) ? (void *)NULL :       \
> +    (void *)((unsigned char *)m->shm_base + offset))

Uh, where is m coming from?  You need to pass that in as well.

> +
> +#define CHUNK_OFFSET(ptr)   ((ptr == (memchunk_t *)NULL) ? 0 :   \
> +    ((unsigned char *)ptr - (unsigned char *)m->list_base))

NULL shouldn't need to be casted to memchunk_t here.  And the fact 
that you are doing casts at all worries me greatly.  I think that 
means that your chosen types are incorrect.  If you are planning on
casting the result of the macro to another type, I think that cast
should be explicit on the part of the code that calls the macro.

> +static memchunk_t *list_malloc (apr_shmem_t *m)

No spaces between malloc and (.

> +{
> +    apr_off_t index;
> +    memlist_t *elem, *last;
> +
> +    /* Find the first free list element */
> +    if ((elem = LIST_ADDR(m->offsets->l_free)) == (memlist_t *)NULL)
> +        return (memchunk_t *)NULL;
> +    else {
> +        index = m->offsets->l_free;
> +        m->offsets->l_free = elem->next;
> +    }
> +
> +    elem->next = -1;
> +
> +    if (m->offsets->l_used == -1)
> +        m->offsets->l_used  = index;
> +    else {
> +        last = LIST_ADDR(m->offsets->l_usedlast);
> +        last->next = index;
> +    }
> +
> +     m->offsets->l_usedlast = index;
> +
> +    return (&(elem->chunk));
> +}
> +
> +static apr_status_t list_free (apr_shmem_t *m, memchunk_t *chunk)
> +{
> +    apr_off_t index;
> +    memlist_t *elem, *iter;
> +
> +    /* Find the required element in the allocated list */
> +    iter = LIST_ADDR(m->offsets->l_used);
> +    while (iter && (&(iter->chunk) != chunk)) {
> +        elem  = iter;
> +        index = iter->next;
> +        iter  = LIST_ADDR(iter->next);
> +    }
> +    
> +    if ((!iter) || (&(iter->chunk) != chunk))
> +        return -1;
> +    else
> +        elem->next = iter->next;
> +
> +    iter->next = -1;
> +
> +    if (m->offsets->l_free == -1)
> +        m->offsets->l_free = index;
> +    else {
> +        elem = LIST_ADDR(m->offsets->l_freelast);
> +        elem->next = index;
> +    }
> +
> +    m->offsets->l_freelast = index;
> +
> +    return APR_SUCCESS;
> +}
> +
> +static void add_chunk (apr_shmem_t *m, apr_off_t *list, memchunk_t *blk)
> +{
> +    memchunk_t *elem;
> +    if (*list == -1)
> +        *list = CHUNK_OFFSET(blk);
> +
> +    elem = CHUNK_ADDR(*list);
> +
> +    if (blk == elem){
> +        blk->prev = CHUNK_OFFSET(blk);
> +        blk->next = CHUNK_OFFSET(blk);
> +    } else {
> +        (CHUNK_ADDR(elem->prev))->next = CHUNK_OFFSET(blk);
> +        blk->prev = elem->prev;
> +        blk->next = CHUNK_OFFSET(elem);
> +        elem->prev = CHUNK_OFFSET(blk);
> +    }
> +}
> +
> +static void remove_chunk(apr_shmem_t *m, apr_off_t *list, memchunk_t
*blk)
> +{
> +    memchunk_t *elem;
> +
> +    elem = CHUNK_ADDR(*list);
> +
> +    if ((elem == blk)
> +       && (CHUNK_ADDR(blk->next) == blk) && (blk ==
CHUNK_ADDR(blk->prev))) {
> +        *list = -1;
> +    } else {
> +        (CHUNK_ADDR(blk->next))->prev = blk->prev;
> +        (CHUNK_ADDR(blk->prev))->next = blk->next;
> +        if (elem == blk)
> +            *list = blk->next;
> +    }
> +    blk->next = -1;
> +    blk->prev = -1;
> +}
> +
> +static void split_chunk(
> +        apr_shmem_t *m, apr_off_t *list, memchunk_t *blk, apr_size_t
size)

The correct style would be:

static void split_chunk(apr_shmem_t *m, apr_off_t *list, memchunk_t *blk, 
                        apr_size_t size)

> +{
> +    memchunk_t *b;
> +
> +    b = list_malloc (m);
> +    if (b != (memchunk_t *)NULL) {

Why the casts to memchunk_t* for NULL?  Just do:

if (b)

> +        b->size   = blk->size - size;
> +        b->offset = blk->offset + size;
> +        blk->size = size;
> +        add_chunk(m, list, b);
> +    }
> +}
> +
> +static memchunk_t *find_chunk_by_addr(
> +        apr_shmem_t *m, apr_off_t *list, void *addr)
> +{
> +    memchunk_t *iter = CHUNK_ADDR(*list);
> +
> +    if (!iter) return NULL;
> +
> +    do {
> +        if (SHM_ADDR(iter->offset) == addr)
> +            return iter;
> +    } while (iter && ((iter = CHUNK_ADDR(iter->next)) !=
CHUNK_ADDR(*list)));
> +
> +    return NULL;
> +}
> +
> +static memchunk_t *find_chunk_by_size(
> +        apr_shmem_t *m, apr_off_t *list, apr_size_t size)
> +{
> +    apr_size_t diff = -1;
> +    memchunk_t *iter = CHUNK_ADDR(*list), *found = NULL;
> +
> +    if (!iter) return NULL;
> +
> +    do {
> +        if (iter->size == size)
> +            return iter;
> +        if (iter->size > size) {
> +            if (diff == -1)
> +                diff = iter->size;
> +            if ((iter->size - size) < diff) {
> +                diff = iter->size - size;
> +                found = iter;
> +            }
> +        }
> +    } while ((iter = CHUNK_ADDR(iter->next)) != CHUNK_ADDR(*list));

This search goes against how the pool code does memory management.
In case you haven't read the pool code, what it does is keeps the
last chunk free and the rest are not-free.  In high alloc/free
usage with lots of fragmentation, this code is going to be horrendously
expensive (think what happens when you have lots of 200-byte chunks
in a 1MB shared memory segment).  Yuck.

> +
> +    if (diff > MIN_BLK_SIZE)
> +        split_chunk(m, list, found, size);
> +
> +    return found;
> +}
> +
> +static void display_shm_snapshot(apr_shmem_t *m)
> +{
> +#if 0
> +    memchunk_t *iter;
> +
> +    extern ap_log_error ();
> +    if (!m) return;
> +
> +    if ((iter = CHUNK_ADDR(m->offsets->c_used)) != NULL) {
> +        ap_log_error(__FILE__, __LINE__, 3, 0, NULL, "USELIST :");
> +        do {
> +            ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"[%ld] [%ld]
[%ld]",
> +                                iter->offset, iter->next, iter->size);
> +        } while (iter &&
> +           ((iter = CHUNK_ADDR(iter->next)) !=
CHUNK_ADDR(m->offsets->c_used)));
> +    }
> +
> +    if ((iter = CHUNK_ADDR(m->offsets->c_free)) != NULL) {
> +        ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"FREELIST :");
> +        do {
> +            ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"[%ld] [%ld]
[%ld]",
> +                                iter->offset, iter->next, iter->size);
> +        } while (iter &&
> +           ((iter = CHUNK_ADDR(iter->next)) !=
CHUNK_ADDR(m->offsets->c_free)));
> +    }
> +    ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"");
> +#endif
> +    return;
> +}

Please no debugging code.  Either the code works, or it doesn't.  If it 
doesn't, then you fix it (and probably before it even gets committed).  
But, the debugging code does not belong in CVS, IMHO.

> +
> +static memchunk_t *alloc_chunk(apr_shmem_t *m, apr_size_t size)
> +{
> +    memchunk_t *b = NULL;
> +
> +    /* Align size to a word boundary */
> +    if (size < MIN_BLK_SIZE)
> +        size = MIN_BLK_SIZE;
> +    size = ((1 + ((size - 1) / sizeof (void *))) * sizeof (void *));
> +
> +    if (m->offsets->shm_length < size)
> +        return NULL;
> +
> +    b = find_chunk_by_size (m, &(m->offsets->c_free), size);
> +
> +    if (b != (memchunk_t *)NULL)
> +        remove_chunk (m, &(m->offsets->c_free), b);
> +    else {
> +        b = list_malloc(m);
> +        if (b == (memchunk_t *)NULL)
> +            return (memchunk_t *)NULL;
> +        b->offset      = m->offsets->shm_offset;
> +        b->size        = size;
> +        m->offsets->shm_offset += b->size;
> +    }
> +
> +    m->offsets->shm_length -= b->size;
> +    add_chunk(m, &(m->offsets->c_used), b);
> +
> +    display_shm_snapshot (m);
> +
> +    return b;
> +}
> +
> +static void free_chunk(apr_shmem_t *m, void *entity)
> +{
> +    memchunk_t *b;
> +
> +    if (entity == NULL)
> +        return;
> +
> +    b = find_chunk_by_addr (m, &(m->offsets->c_used), entity);
> +    if (b != (memchunk_t *)NULL) {
> +        remove_chunk(m, &(m->offsets->c_used), b);
> +        add_chunk(m, &(m->offsets->c_free), b);
> +        m->offsets->shm_length += b->size;
> +    }
> +
> +    display_shm_snapshot (m);
> +}
> +
> +static memchunk_t *realloc_chunk(apr_shmem_t *m, void *entity, apr_size_t
size)
> +{
> +    memchunk_t *b, *new_b;
> +
> +    /* Align size to a word boundary */
> +    size = ((1 + ((size - 1) / sizeof (void *))) * sizeof (void *));
> +
> +    b = find_chunk_by_addr(m, &(m->offsets->c_used), entity);
> +
> +    if (b != (memchunk_t *)NULL) {
> +        if (b->size > size)
> +            split_chunk (m, &(m->offsets->c_used), b, size);
> +        else
> +        if ((b->size < size) && (size < m->offsets->shm_length))
{
> +            new_b = alloc_chunk (m, size);
> +            memcpy (SHM_ADDR(new_b->offset), SHM_ADDR(b->offset),
b->size);
> +            free_chunk (m, entity);
> +            b = new_b;
> +        }
> +    }
> +    return b;
> +}
> +
> +/* 
> + * FIXME: 
> + * 1. Is APR_OS_DEFAULT sufficient? 
> + * 2. status = apr_file_remove(filename, p);
> + * 3. Handle errors from return values of system calls.
> + */

You moved the FIXMEs from the code itself (where the context was
clearer) to the top of the function where the context has been lost.
Please restore them to their proper place - this is how we typically
record FIXMEs - as close to where the questionable code actually is.

>  APR_DECLARE(apr_status_t) apr_shm_init(apr_shmem_t **m, apr_size_t
reqsize, 
> -                                       const char *filename, apr_pool_t
*pool)
> +                                       const char *filename, apr_pool_t
*p)

Why the change from pool to p?  I don't see a reason for that.

>  {
> -    apr_shmem_t *new_m;
> -    void *mem;
>  #if APR_USE_SHMEM_SHMGET
>      struct shmid_ds shmbuf;
>      apr_uid_t uid;
>      apr_gid_t gid;
> +    int i, listsize;
> +    memlist_t *iter, *piter;
> +    void *addr;
>  #endif
> +
>  #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM ||
APR_USE_SHMEM_MMAP_ZERO
>      apr_status_t status;
> +    status = APR_SUCCESS;
>  #endif
> +
>  #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM || \
>      APR_USE_SHMEM_MMAP_ZERO || APR_USE_SHMEM_SHMGET
>      int tmpfd;
>  #endif
>  
> -    new_m = apr_palloc(pool, sizeof(apr_shmem_t));
> -    if (!new_m)
> +    (*m) = (apr_shmem_t *)apr_palloc(p, sizeof(apr_shmem_t));

You shouldn't modify m until you know you have succeeded (i.e. when you
are about ready to return success).  Also, the (*m) serves to make the
code much less readable and actually slower since you might have to
do derefencing a lot more.  Please restore the new_m.  It also makes the
diff considerably less.

> +    if (!(*m))
>          return APR_ENOMEM;
>  
> -/* These implementations are very similar except for opening the file. */
>  #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM ||
APR_USE_SHMEM_MMAP_ZERO
> -    /* FIXME: Ignore error for now. *
> -     * status = apr_file_remove(filename, pool);*/
> -    status = APR_SUCCESS;
>      
>  #if APR_USE_SHMEM_MMAP_TMP
> -    /* FIXME: Is APR_OS_DEFAULT sufficient? */
> -    status = apr_file_open(&new_m->file, filename, 
> -                         APR_READ | APR_WRITE | APR_CREATE,
APR_OS_DEFAULT,
> -                         pool);
> +    status = apr_file_open(&(*m)->file, filename, 
> +                         APR_READ | APR_WRITE | APR_CREATE,
APR_OS_DEFAULT, p);
>      if (status != APR_SUCCESS)
>          return APR_EGENERAL;
>  
> -    status = apr_os_file_get(&tmpfd, new_m->file);
> -    status = apr_file_trunc(new_m->file, reqsize);
> +    status = apr_os_file_get(&tmpfd, (*m)->file);
> +    status = apr_file_trunc((*m)->file, reqsize);
>      if (status != APR_SUCCESS)
>          return APR_EGENERAL;
>  
>  #elif APR_USE_SHMEM_MMAP_SHM
> -    /* FIXME: Is APR_OS_DEFAULT sufficient? */
>      tmpfd = shm_open(filename, O_RDWR | O_CREAT, APR_OS_DEFAULT);
>      if (tmpfd == -1)
>          return errno;
>  
> -    apr_os_file_put(&new_m->file, &tmpfd, pool); 
> -    status = apr_file_trunc(new_m->file, reqsize);
> +    apr_os_file_put(&(*m)->file, &tmpfd, p); 
> +    status = apr_file_trunc((*m)->file, reqsize);
>      if (status != APR_SUCCESS)
>      {
>          shm_unlink(filename);
>          return APR_EGENERAL;
>      }
>  #elif APR_USE_SHMEM_MMAP_ZERO
> -    status = apr_file_open(&new_m->file, "/dev/zero", APR_READ |
APR_WRITE, 
> -                         APR_OS_DEFAULT, pool);
> +    status = apr_file_open(&(*m)->file, "/dev/zero", APR_READ |
APR_WRITE, 
> +                         APR_OS_DEFAULT, p);
>      if (status != APR_SUCCESS)
>          return APR_EGENERAL;
> -    status = apr_os_file_get(&tmpfd, new_m->file);
> +    status = apr_os_file_get(&tmpfd, (*m)->file);
>  #endif
>  
> -    mem = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, MAP_SHARED, tmpfd,
0);
> +    (*m)->shm_base = mmap(NULL, reqsize, PROT_READ|PROT_WRITE,
> +                                                    MAP_SHARED, tmpfd,
0);
>  
>  #elif APR_USE_SHMEM_MMAP_ANON
> -    mem = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED,
-1, 0);
> +    (*m)->shm_base = mmap(NULL, reqsize, PROT_READ|PROT_WRITE,
> +
MAP_ANON|MAP_SHARED,-1, 0);
>  #elif APR_USE_SHMEM_SHMGET
> -    tmpfd = shmget(IPC_PRIVATE, reqsize, (SHM_R|SHM_W|IPC_CREAT));
> -    if (tmpfd == -1)
> -        return errno;
>  
> -    new_m->file = tmpfd;
> +    listsize = (reqsize/MIN_BLK_SIZE + 1) * sizeof(memlist_t) 
> +                     + sizeof (memoffsets_t);
> +    (*m)->listfd = shmget(IPC_PRIVATE, listsize,
(SHM_R|SHM_W|IPC_CREAT));
> +    if ((*m)->listfd == -1)
> +        return errno + APR_OS_START_SYSERR;

Eh, what?  It looks like this whole logic is only available if you are
using shmget.  No current OS actually uses shmget anymore - MAP_ANON
should be our best choice.  

> +
> +    addr = (void *)shmat((*m)->listfd, NULL, 0);
> +    if (addr == (void *)NULL)
> +        return errno + APR_OS_START_SYSERR;
> +
> +    (*m)->offsets      = (memoffsets_t *)addr;
> +    (*m)->list_base = (memlist_t *)(addr + (sizeof(memoffsets_t)));
> +
> +    (*m)->file = shmget(IPC_PRIVATE, reqsize, (SHM_R|SHM_W|IPC_CREAT));
> +    if ((*m)->file == -1)
> +        return errno + APR_OS_START_SYSERR;
> +
> +    (*m)->shm_base = (void *)shmat((*m)->file, NULL, 0);
> +    if ((*m)->shm_base == (void *)NULL)
> +        return errno + APR_OS_START_SYSERR;
>  
> -    mem = shmat(new_m->file, NULL, 0);
> -
> -    /* FIXME: Handle errors. */
> -    if (shmctl(new_m->file, IPC_STAT, &shmbuf) == -1)
> +    if (shmctl((*m)->file, IPC_STAT, &shmbuf) == -1)
>          return errno;
>  
> -    apr_current_userid(&uid, &gid, pool);
> +    apr_current_userid(&uid, &gid, p);
>      shmbuf.shm_perm.uid = uid;
>      shmbuf.shm_perm.gid = gid;
>  
> -    if (shmctl(new_m->file, IPC_SET, &shmbuf) == -1)
> -        return errno;
> +    if (shmctl((*m)->file, IPC_SET, &shmbuf) == -1)
> +        return errno + APR_OS_START_SYSERR;
>  
> -    /* remove in future (once use count hits zero) */
> -    if (shmctl(new_m->file, IPC_RMID, NULL) == -1)
> -        return errno;
> +    if (shmctl((*m)->listfd, IPC_SET, &shmbuf) == -1)
> +        return errno + APR_OS_START_SYSERR;
>  
> +    /* Initially all elements after the first element is a free block */
> +    piter = (*m)->list_base;
> +    for (i = 0; i < reqsize/MIN_BLK_SIZE; i++) {
> +        piter->next = i + 1;
> +        piter++;
> +    }
> +    piter->next = -1;
> +
> +    (*m)->offsets->l_used     = -1;
> +    (*m)->offsets->l_usedlast = -1;
> +    (*m)->offsets->l_free     = 0;
> +    (*m)->offsets->l_freelast = i;
> +    (*m)->offsets->c_used     = -1;
> +    (*m)->offsets->c_free     = -1;
> +
>  #elif APR_USE_SHMEM_BEOS
> -    new_m->area_id = create_area("mm", (void*)&mem, B_ANY_ADDRESS,
reqsize, 
> -                                 B_LAZY_LOCK, B_READ_AREA|B_WRITE_AREA);
> -    /* FIXME: error code? */
> -    if (new_m->area_id < 0)
> -        return APR_EGENERAL;
>  
> +    (*m)->area_id = create_area("apr_shm", (void*)&((*m)->shm_base),
B_ANY_ADDRESS,
> +                                reqsize, B_LAZY_LOCK,
B_READ_AREA|B_WRITE_AREA);
> +    if ((*m)->area_id < 0)
> +        return APR_EGENERAL; /* FIXME: error code? */
>  #endif
>  
> -    new_m->mem = mem;
> -    new_m->curmem = mem;
> -    new_m->length = reqsize;
> +    (*m)->p = p;
> +    (*m)->offsets->shm_offset = 0;
> +    (*m)->offsets->shm_length = reqsize;
>  
> -    apr_lock_create(&new_m->lock, APR_MUTEX, APR_CROSS_PROCESS, NULL,
pool);
> -    if (!new_m->lock)
> +    apr_lock_create(&(*m)->lock, APR_MUTEX, APR_CROSS_PROCESS, NULL, p);
> +    if (!(*m)->lock)
>          return APR_EGENERAL;
>  
> -    *m = new_m; 
>      return APR_SUCCESS;
>  }

At this point, it is obvious to me that this code only works with
shmget (there is no extra allocation of the list structures for 
mmap()-based shared memory systems).  I could be wrong, but I'm seeing
no way that the list structures are going to be allocated in shared
memory for anything other than shmget.  (Or, you are using part of
the memory that was originally allocated to store the list structures).

If your intention is to support all shared memory, please submit a 
patch that does so.  If you do not intend to support the other 
systems (and it'll only work with shmget), then this does not warrant 
inclusion into APR.  -- justin

Mime
View raw message