On Sat, Jan 27, 2018 at 4:41 PM, Yann Ylavic <[email protected]> wrote:
> On Sat, Jan 27, 2018 at 4:39 PM, Yann Ylavic <[email protected]> wrote:
>> On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <[email protected]> wrote:
>>>
>>> So I wonder if we'd better:
>>> 1. have a constant file name (no generation suffix) for all systems,
>>> 2. not unlink/remove the SHM file on pconf cleanup,
>>> 2'. eventually unlink the ones unused in ++generation (e.g. retained
>>> global list),
>>> 3. unlink all on pglobal cleanup.
>>>
>>> Now we'd have a working shm_attach() not only for persisted slotmems,
>>> while shm_create() would only be called for new SHMs which we know can
>>> be pre-removed (to work around crashes leaving them registered
>>> somewhere).
>>> Also, if attach succeeds on startup (first generation) but the SHM is
>>> not persisted (an old crash still), we can possibly pass the sizes
>>> checks and re-create the SHM regardless.
>>>
>>> WDYT?
>>
>> Something like the attached patch (it talks better sometimes...).
>> Not even tested of course :)
>
> Oops, more than needed in there, here is the good one.
OK, v2 already, with correct usage of gpool (i.e. pconf), distinct
from ap_pglobal.
Index: modules/slotmem/mod_slotmem_shm.c
===================================================================
--- modules/slotmem/mod_slotmem_shm.c (revision 1821739)
+++ modules/slotmem/mod_slotmem_shm.c (working copy)
@@ -47,7 +47,6 @@ struct ap_slotmem_instance_t {
int fbased; /* filebased? */
void *shm; /* ptr to memory segment (apr_shm_t *) */
void *base; /* data set start */
- apr_pool_t *gpool; /* per segment global pool */
char *inuse; /* in-use flag table*/
unsigned int *num_free; /* slot free count for this instance */
void *persist; /* persist dataset start */
@@ -64,7 +63,8 @@ struct ap_slotmem_instance_t {
*/
/* global pool and list of slotmem we are handling */
-static struct ap_slotmem_instance_t *globallistmem = NULL;
+static struct ap_slotmem_instance_t *globallistmem = NULL,
+ **retained_globallistmem = NULL;
static apr_pool_t *gpool = NULL;
#define DEFAULT_SLOTMEM_PREFIX "slotmem-shm-"
@@ -191,11 +191,11 @@ static void store_slotmem(ap_slotmem_instance_t *s
if (storename) {
rv = apr_file_open(&fp, storename, APR_CREATE | APR_READ | APR_WRITE,
- APR_OS_DEFAULT, slotmem->gpool);
+ APR_OS_DEFAULT, gpool);
if (APR_STATUS_IS_EEXIST(rv)) {
- apr_file_remove(storename, slotmem->gpool);
+ apr_file_remove(storename, gpool);
rv = apr_file_open(&fp, storename, APR_CREATE | APR_READ | APR_WRITE,
- APR_OS_DEFAULT, slotmem->gpool);
+ APR_OS_DEFAULT, gpool);
}
if (rv != APR_SUCCESS) {
return;
@@ -212,7 +212,7 @@ static void store_slotmem(ap_slotmem_instance_t *s
}
apr_file_close(fp);
if (rv != APR_SUCCESS) {
- apr_file_remove(storename, slotmem->gpool);
+ apr_file_remove(storename, gpool);
}
}
}
@@ -271,26 +271,35 @@ static apr_status_t restore_slotmem(void *ptr, con
return rv;
}
+static apr_status_t global_cleanup_slotmem(void *param)
+{
+ AP_DEBUG_ASSERT(retained_globallistmem != NULL);
+ *retained_globallistmem = NULL;
+
+ while (globallistmem) {
+ if (globallistmem->fbased) {
+ apr_shm_destroy(globallistmem->shm);
+ apr_shm_remove(globallistmem->name, ap_pglobal);
+ apr_file_remove(globallistmem->name, ap_pglobal);
+ }
+ globallistmem = globallistmem->next;
+ }
+
+ return APR_SUCCESS;
+}
+
static apr_status_t cleanup_slotmem(void *param)
{
- ap_slotmem_instance_t **mem = param;
+ AP_DEBUG_ASSERT(retained_globallistmem != NULL);
+ *retained_globallistmem = globallistmem;
- if (*mem) {
- ap_slotmem_instance_t *next = *mem;
- while (next) {
- if (AP_SLOTMEM_IS_PERSIST(next)) {
- store_slotmem(next);
- }
- apr_shm_destroy((apr_shm_t *)next->shm);
- if (next->fbased) {
- apr_shm_remove(next->name, next->gpool);
- apr_file_remove(next->name, next->gpool);
- }
- next = next->next;
+ while (globallistmem) {
+ if (AP_SLOTMEM_IS_PERSIST(globallistmem)) {
+ store_slotmem(globallistmem);
}
+ globallistmem = globallistmem->next;
}
- /* apr_pool_destroy(gpool); */
- globallistmem = NULL;
+
return APR_SUCCESS;
}
@@ -338,11 +347,11 @@ static apr_status_t slotmem_create(ap_slotmem_inst
apr_size_t size = AP_SLOTMEM_OFFSET + AP_UNSIGNEDINT_OFFSET +
(item_num * sizeof(char)) + basesize;
int persist = (type & AP_SLOTMEM_TYPE_PERSIST) != 0;
+ int generation = 0;
apr_status_t rv;
- if (gpool == NULL) {
- return APR_ENOSHMAVAIL;
- }
+ ap_mpm_query(AP_MPMQ_GENERATION, &generation);
+
if (slotmem_filenames(pool, name, &fname, persist ? &pname : NULL)) {
/* first try to attach to existing slotmem */
if (next) {
@@ -372,13 +381,13 @@ static apr_status_t slotmem_create(ap_slotmem_inst
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02300)
"create %s: %"APR_SIZE_T_FMT"/%u", fname, item_size,
item_num);
- if (fbased) {
- rv = apr_shm_attach(&shm, fname, gpool);
+ if (fbased && generation > 1) {
+ rv = apr_shm_attach(&shm, fname, pool);
}
else {
rv = APR_EINVAL;
}
- if (rv == APR_SUCCESS) {
+ if (rv == APR_SUCCESS && persist) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02598)
"apr_shm_attach() succeeded");
@@ -401,14 +410,14 @@ static apr_status_t slotmem_create(ap_slotmem_inst
}
ptr += AP_SLOTMEM_OFFSET;
}
- else {
+ else if (rv != APR_SUCCESS) {
apr_size_t dsize = size - AP_SLOTMEM_OFFSET;
if (fbased) {
- apr_shm_remove(fname, gpool);
- rv = apr_shm_create(&shm, size, fname, gpool);
+ apr_shm_remove(fname, pool);
+ rv = apr_shm_create(&shm, size, fname, ap_pglobal);
}
else {
- rv = apr_shm_create(&shm, size, NULL, gpool);
+ rv = apr_shm_create(&shm, size, NULL, ap_pglobal);
}
ap_log_error(APLOG_MARK, rv == APR_SUCCESS ? APLOG_DEBUG : APLOG_ERR,
rv, ap_server_conf, APLOGNO(02611)
@@ -444,10 +453,9 @@ static apr_status_t slotmem_create(ap_slotmem_inst
}
/* For the chained slotmem stuff */
- res = (ap_slotmem_instance_t *) apr_pcalloc(gpool,
- sizeof(ap_slotmem_instance_t));
- res->name = apr_pstrdup(gpool, fname);
- res->pname = apr_pstrdup(gpool, pname);
+ res = apr_pcalloc(ap_pglobal, sizeof(ap_slotmem_instance_t));
+ res->name = apr_pstrdup(ap_pglobal, fname);
+ res->pname = apr_pstrdup(ap_pglobal, pname);
res->fbased = fbased;
res->shm = shm;
res->num_free = (unsigned int *)ptr;
@@ -458,7 +466,6 @@ static apr_status_t slotmem_create(ap_slotmem_inst
ptr += AP_UNSIGNEDINT_OFFSET;
res->base = (void *)ptr;
res->desc = desc;
- res->gpool = gpool;
res->next = NULL;
res->inuse = ptr + basesize;
if (globallistmem == NULL) {
@@ -485,9 +492,6 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
apr_shm_t *shm;
apr_status_t rv;
- if (gpool == NULL) {
- return APR_ENOSHMAVAIL;
- }
if (!slotmem_filenames(pool, name, &fname, NULL)) {
return APR_ENOSHMAVAIL;
}
@@ -517,7 +521,7 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
}
/* next try to attach to existing shared memory */
- rv = apr_shm_attach(&shm, fname, gpool);
+ rv = apr_shm_attach(&shm, fname, ap_pglobal);
if (rv != APR_SUCCESS) {
return rv;
}
@@ -528,9 +532,8 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
ptr += AP_SLOTMEM_OFFSET;
/* For the chained slotmem stuff */
- res = (ap_slotmem_instance_t *) apr_pcalloc(gpool,
- sizeof(ap_slotmem_instance_t));
- res->name = apr_pstrdup(gpool, fname);
+ res = apr_pcalloc(ap_pglobal, sizeof(ap_slotmem_instance_t));
+ res->name = apr_pstrdup(ap_pglobal, fname);
res->fbased = 1;
res->shm = shm;
res->num_free = (unsigned int *)ptr;
@@ -538,7 +541,6 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
ptr += AP_UNSIGNEDINT_OFFSET;
res->base = (void *)ptr;
res->desc = desc;
- res->gpool = gpool;
res->inuse = ptr + (desc.size * desc.num);
res->next = NULL;
if (globallistmem == NULL) {
@@ -760,18 +762,22 @@ static const ap_slotmem_provider_t *slotmem_shm_ge
}
/* initialise the global pool */
-static void slotmem_shm_initgpool(apr_pool_t *p)
+static int pre_config(apr_pool_t *p, apr_pool_t *plog,
+ apr_pool_t *ptemp)
{
- gpool = p;
+ const char *retained_key = "mod_slotmem_shm";
+ retained_globallistmem = ap_retained_data_get(retained_key);
+ if (!retained_globallistmem) {
+ retained_globallistmem =
+ ap_retained_data_create(retained_key,
+ sizeof *retained_globallistmem);
+ apr_pool_cleanup_register(ap_pglobal, NULL, global_cleanup_slotmem,
+ apr_pool_cleanup_null);
+ }
+ globallistmem = *retained_globallistmem;
+ return OK;
}
-/* Add the pool_clean routine */
-static void slotmem_shm_initialize_cleanup(apr_pool_t *p)
-{
- apr_pool_cleanup_register(p, &globallistmem, cleanup_slotmem,
- apr_pool_cleanup_null);
-}
-
/*
* Make sure the shared memory is cleaned
*/
@@ -778,17 +784,12 @@ static const ap_slotmem_provider_t *slotmem_shm_ge
static int post_config(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp,
server_rec *s)
{
- slotmem_shm_initialize_cleanup(p);
+ gpool = p;
+ apr_pool_cleanup_register(p, NULL, cleanup_slotmem,
+ apr_pool_cleanup_null);
return OK;
}
-static int pre_config(apr_pool_t *p, apr_pool_t *plog,
- apr_pool_t *ptemp)
-{
- slotmem_shm_initgpool(p);
- return OK;
-}
-
static void ap_slotmem_shm_register_hook(apr_pool_t *p)
{
const ap_slotmem_provider_t *storage = slotmem_shm_getstorage();
