From a0b44e9256655df3f7450a6440b14363ee2c1406 Mon Sep 17 00:00:00 2001 From: Miklos Tirpak Date: Fri, 8 Feb 2008 17:09:45 +0000 Subject: [PATCH] - Call the per-child process callback functions even if the config variables are changed before forking, so the modules/core will not miss the change. - fixing a very unlikely bug: when a module kept forking and destroying new processes runtime (only jabber module does so), and two variables were changed after the forked process called called cfg_update() and before exited, and both variables had per-child process callback, and all the other child processes updated their own local config faster then this one, the list of the callbacks was not released, thus the memory was not freed. --- cfg/cfg_ctx.c | 21 +++++++++------ cfg/cfg_struct.c | 67 +++++++++++++++++++++++++++++++++++------------- cfg/cfg_struct.h | 7 +++++ 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/cfg/cfg_ctx.c b/cfg/cfg_ctx.c index eac562a256..df7458dbbd 100644 --- a/cfg/cfg_ctx.c +++ b/cfg/cfg_ctx.c @@ -303,16 +303,16 @@ int cfg_set_now(cfg_ctx_t *ctx, str *group_name, str *var_name, } - if (cfg_shmized) { - if (var->def->on_set_child_cb) { - child_cb = cfg_child_cb_new(var_name, - var->def->on_set_child_cb); - if (!child_cb) { - LOG(L_ERR, "ERROR: cfg_set_now(): not enough shm memory\n"); - goto error0; - } + if (var->def->on_set_child_cb) { + child_cb = cfg_child_cb_new(var_name, + var->def->on_set_child_cb); + if (!child_cb) { + LOG(L_ERR, "ERROR: cfg_set_now(): not enough shm memory\n"); + goto error0; } + } + if (cfg_shmized) { /* make sure that nobody else replaces the global config while the new one is prepared */ CFG_WRITER_LOCK(); @@ -381,6 +381,11 @@ int cfg_set_now(cfg_ctx_t *ctx, str *group_name, str *var_name, /* flag the variable because there is no need to shmize it again */ var->flag |= cfg_var_shmized; + + /* the global config does not have to be replaced, + but the child callback has to be installed, otherwise the + child processes will miss the change */ + cfg_install_child_cb(child_cb, child_cb); } if (val_type == CFG_VAR_INT) diff --git a/cfg/cfg_struct.c b/cfg/cfg_struct.c index 25e9c54ae9..1eca9777b0 100644 --- a/cfg/cfg_struct.c +++ b/cfg/cfg_struct.c @@ -385,28 +385,50 @@ int cfg_child_init(void) */ void cfg_child_destroy(void) { + cfg_child_cb_t *prev_cb; + /* unref the local config */ if (cfg_local) { CFG_UNREF(cfg_local); cfg_local = NULL; } - /* unref the per-process callback list */ - if (atomic_dec_and_test(&cfg_child_cb->refcnt)) { - /* No more pocess refers to this callback. - Did this process block the deletion, - or is there any other process that has not - reached prev_cb yet? */ - CFG_LOCK(); - if (*cfg_child_cb_first == cfg_child_cb) { - /* yes, this process was blocking the deletion */ - *cfg_child_cb_first = cfg_child_cb->next; - CFG_UNLOCK(); - shm_free(cfg_child_cb); + if (!cfg_child_cb) return; + + /* The lock must be held to make sure that the global config + is not replaced meantime, and the other child processes do not + leave the old value of *cfg_child_cb_last. Otherwise it could happen, + that all the other processes move their own cfg_child_cb pointer before + this process reaches *cfg_child_cb_last, though, it is very unlikely. */ + CFG_LOCK(); + + /* go through the list and check whether there is any item that + has to be freed (similar to cfg_update_local(), but without executing + the callback functions) */ + while (cfg_child_cb != *cfg_child_cb_last) { + prev_cb = cfg_child_cb; + cfg_child_cb = cfg_child_cb->next; + atomic_inc(&cfg_child_cb->refcnt); + if (atomic_dec_and_test(&prev_cb->refcnt)) { + /* No more pocess refers to this callback. + Did this process block the deletion, + or is there any other process that has not + reached prev_cb yet? */ + if (*cfg_child_cb_first == prev_cb) { + /* yes, this process was blocking the deletion */ + *cfg_child_cb_first = cfg_child_cb; + shm_free(prev_cb); + } } else { - CFG_UNLOCK(); + /* no need to continue, because there is at least + one process that stays exactly at the same point + in the list, so it will free the items later */ + break; } } + atomic_dec(&cfg_child_cb->refcnt); + + CFG_UNLOCK(); cfg_child_cb = NULL; } @@ -480,6 +502,18 @@ cfg_block_t *cfg_clone_global(void) return block; } +/* append new callbacks to the end of the child callback list + * + * WARNING: the function is unsafe, either hold CFG_LOCK(), + * or call the function before forking + */ +void cfg_install_child_cb(cfg_child_cb_t *cb_first, cfg_child_cb_t *cb_last) +{ + /* add the new callbacks to the end of the linked-list */ + (*cfg_child_cb_last)->next = cb_first; + *cfg_child_cb_last = cb_last; +} + /* installs a new global config * * replaced is an array of strings that must be freed together @@ -499,11 +533,8 @@ void cfg_install_global(cfg_block_t *block, char **replaced, CFG_REF(block); *cfg_global = block; - if (cb_first) { - /* add the new callbacks to the end of the linked-list */ - (*cfg_child_cb_last)->next = cb_first; - *cfg_child_cb_last = cb_last; - } + if (cb_first) + cfg_install_child_cb(cb_first, cb_last); CFG_UNLOCK(); diff --git a/cfg/cfg_struct.h b/cfg/cfg_struct.h index 3afca59411..95af1b7eb0 100644 --- a/cfg/cfg_struct.h +++ b/cfg/cfg_struct.h @@ -262,6 +262,13 @@ cfg_block_t *cfg_clone_global(void); /* clones a string to shared memory */ int cfg_clone_str(str *src, str *dst); +/* append new callbacks to the end of the child callback list + * + * WARNING: the function is unsafe, either hold CFG_LOCK(), + * or call the function before forking + */ +void cfg_install_child_cb(cfg_child_cb_t *cb_first, cfg_child_cb_t *cb_last); + /* installs a new global config * * replaced is an array of strings that must be freed together -- 2.20.1