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).
Comments (non-critical stuff):
1. open_memstream() doesn't look very portable
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:
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
