[asterisk-dev] bug 6002, new Algorithm for merge_contexts_and_delete

[Watkins, Bradley]

> -----Original Message-----
> From: asterisk-dev-bounces at lists.digium.com 
> [mailto:asterisk-dev-bounces at lists.digium.com] On Behalf Of 
> Steve Murphy
> Sent: Thursday, March 01, 2007 2:02 PM
> To: asterisk-dev at lists.digium.com
> Subject: [asterisk-dev] bug 6002, new Algorithm for 
> merge_contexts_and_delete
> 
> I want folks to review this before I implement it. Hate to do 
> all this work, and end up with a lemon.
> 
> Please read bug 6002, which was introduced by rizzo last 
> year, and try to understand the shortcomings of the current 
> merge_contexts_and_delete, when it comes to regcontext; and 
> see if the below algorithm will meet the needs.
> 
> I've posted the text below to bug 6002, but I think it would 
> be good to have the community as a whole review the proposed 
> algorithm, and let me know if it's flawed.
> 
> (Also, it won't hurt to look at bug 8574-- Call processing 
> stops while reloading large dialpan)
> 
> OK, been looking at the code. I have to add one more requirement to
> merge_contexts_and_delete: that it hold the locks for less 
> than a frame time. Since the freeing and destruction of list 
> elements is the most time-consuming part of the operation, 
> (or, at least, WAS), I propose this algorithm:
> 
> 4 lists are involved:
>     1. the list of contexts to merge into the dialplan (extcontexts)
>     2. the existing contexts (contexts)
>     3. a list containing extens to free
>     4. a list containing just contexts to free
> 
> The algorithm would go something like this:
>     1. get the conlock & hintlock
>     2. preserve the watchers as before
>     3. traverse the dp, and unlink all exten/prio that match 
> registrar.
> Do Not
>        remove any contexts (yet). Unlink them from the 
> contexts list, and
>        link them instead to the list of extens to free.
>     4. traverse the dp again, and for any empty contexts, 
> that match registrar,
>        unlink from contexts, and link to the contexts to free 
> list. this and #3
>        might be tied into a single traversal.
>     5. Now, for each context in extcontexts, search for a 
> match in contexts.
>          (THIS MAKES ME NERVOUS IF THE DP IS BIG!)
>        if found:
>          either the context or something in it has a 
> different registrar.
>          go thru the contexts entry, and relink the 
> exten/prios into the
>          matching extcontext's entry. If there are exten
>          collisions, (THIS SEARCH MAKES ME NERVOUS IF THE DP IS BIG!) 
>          then take all the contexts prios for that exten, and 
> insert them into
>          the collided extcontexts exten. Issue a warning only 
> if there are
>          collisions. Keep the extcontexts version. After all 
> the prios are
>          merged, then put the contexts exten into the free list.
> 
>          Now, Move the now empty contexts' context into the 
> free context list.
>          Then move the merged context into the contexts list from the
>          extcontext list. 
>        if not found:
>          link the context from extcontexts to contexts. This 
> is the "quick and 
>          easy" path.
> 
>     6. Restore the watchers, as is now being done.
>     
>     7. Unlock the above locks,
> 
>     8. Destroy the stuff in the to-be-freed lists
> 
>     9. Return.
> 
> This will keep the regcontexts. If the dp is big, or the 
> extcontexts big, this operation will run dangerously slow! 
> Having O(1) search times for all 3 types of search (context, 
> exten, prio) would be a big plus.
> 
> Will this be sufficient? The key is getting part 5 right.

This all sounds like a lot of work for what amounts to a bug in chan_sip
(that's now fixed, it'll no longer blithely create contexts that are
already there) and a configuration error.

Part of how regcontext works (in the code, not the user configuration
and such) is based on the assumption that it has created and can control
the contexts mentioned in the regcontext global parameter.  At a
minimum, it would affect how this code currently works and should be
modified accordingly.  In fact, not doing so would result in precisely
the opposite of the problem Rizzo reported: if someone removed a context
from the regcontext parameter, it would remove that 'merged' context.
Doing so could result in unnecessary complication to that code, since
now chan_sip has to know a lot more about the extensions and contexts it
has created.

Even the behavior of adding extensions to contexts created by some other
registrar could be weeded out with little effort and code.  It would
also prevent chan_sip's current behavior of removing contexts that it
*thought* it controlled (because it was mentioned in the regcontext
parameter of the config) but did not.


That said, I can't say that I'm particularly qualified to evaluate the
proposal itself.  I just figured I'd chime in on the bug (I can post
these comments to the bug itself as well) and the potential impact your
solution will have on chan_sip.  I also suspect this would affect the
IAX2 implementation of regcontext as well, but I don't know enough about
the code (which I believe is now somewhat different that chan_sip's) to
comment further.

Regards,
- Brad

The contents of this e-mail are intended for the named addressee only. It contains information that may be confidential. Unless you are the named addressee or an authorized designee, you may not copy or use it, or disclose it to anyone else. If you received it in error please notify us immediately and then destroy it.