Andrei Pelinescu-Onciul wrote:
On Dec 04, 2008 at 12:36, Alfred E. Heggestad <[EMAIL PROTECTED]> wrote:
Hi
please find attached a patch for a new module called 'prefix_route'
we want to merge this into the main SER v2.1 CVS, but would prefer
some review comments from the community before it is merged.
the module description can be found in the README file (in the patch).
It looks very good to me. It even has lots of comments and docs :-)
You should commit it (I think more people will review it if it's on
cvs anyway).
thanks for your positive comments :) we have now committed an alternative
version to CVS. please see below:
Comments (non-critical stuff):
1. open_memstream() doesn't look very portable
you are right, open_memstream() requires the _GNU_SOURCE macro, and
this does not build on e.g. MAC OS X. the code is now rewritten in
a more portable way..
2. you could avoid taking a lock every time (in tree_route_get()).
You could "missuse" the cfg framework: declare a pointer config
variable that will point to the tree and access the tree using
tree=cfg_get(...). When you change it (rpc triggered db reload) you
would have to use cfg_set_now(...). See doc/cfg.txt for more info
(you can skip over the callbacks and fixups, you don't need them).
You could also grep for tm_cfg and default_tm_cfg in tm.
This will get rid of locking (locking willbe used only when changing the
config).
Alternatively you could write a better tree_get(), something like:
we had a look at both options, and ended up with only a few minor
changes, including using an atomic_t for the reference counter.
thanks for your comments, more comments are welcome :)
/alfred
struct tree* local_tree; /* local pointer to the shared main tree */
struct tree** main_tree; /* *main_tree == current config tree */
lock_t main_tree_chg;
tree_get(...){
if (local_tree != *main_tree){
/* main tree changed, we have to release our current tree
and update local_tree to point to the new one */
if atomic_dec_and_test(&local_tree->refcnt)
tree_free(local_tree)
lock(main_tree_chg); /* don't allow anyone to modify *main_tree*/
local_tree=*main_tree;
atomic_inc(&local_tree->refcnt); /* get a ref. for our
process */
unlock(main_tree_chg);
/* now nobody will free the tree under us, because we hold a
* ref*/
}
return local_tree;
}
set_new_tree(new_tree){
atomic_inc(new_tree->refcnt); /* or new_tree->refcnt=1 */
lock(main_tree_chg);
old_tree=*main_tree;
atomic_set(*main_tree, new_tree);
unlock(main_tree_chg);
if (atomic_dec_and_test(&old_tree->refcnt)
tree_free(old_tree);
}
This is similar to what the cfg framework does (see cfg_update() and
cfg_update_local()), without all the fixups and callbacks.
Andrei
_______________________________________________
Serdev mailing list
[email protected]
http://lists.iptel.org/mailman/listinfo/serdev
