[asterisk-dev] [Code Review] 2930: pbx.c: eliminate confusion when matching caller id and prevent removal of extension entry still in hashtable

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2930/#review9963
-----------------------------------------------------------


I only was able to make it through the coding guidelines review of this before I had to run.


/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/2930/#comment19207>

    In Asterisk, we have a handy shortcut that prevents you from having to do
    
    if (!foo || !*foo). Instead, you can call ast_strlen_zero(foo) for the same functionality.
    
    So in this case, you can just call
    
    if (ast_strlen_zero(ac) && ast_strlen_zero(bc)) {
        return 0;
    }



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/2930/#comment19208>

    Trailing whitespace here (appears as a red blob on review board).



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/2930/#comment19211>

    Coding Guidelines: put a space after the comma.



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/2930/#comment19209>

    Coding guidelines: Put a space on either side of the == operator.



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/2930/#comment19210>

    Coding Guidelines: remove the space between "cidmatch" and the comma.


- Mark Michelson


On Oct. 18, 2013, 5:35 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2930/
> -----------------------------------------------------------
> 
> (Updated Oct. 18, 2013, 5:35 p.m.)
> 
> 
> Review request for Asterisk Developers and Guenther Kelleter.
> 
> 
> Bugs: AST-1235
>     https://issues.asterisk.org/jira/browse/AST-1235
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes a bug report where restarting Asterisk repeatedly would cause a crash due to accidentally deleting extensions that were still referenced in the context's root_table.  This occurred due to confusion over matching or not matching extensions based on callerid, which is also being cleared up via an enum'd third state.
> 
> There are two values in an extension entry:  matchcid which says whether cid should be matched, and cidmatch which is the string value to match.  The matchcid value was previously non-zero when cidmatch was signficant.  This is important since a null length callerid match must be kept distinct from a match-anything condition.
> 
> This patch adds an enum to define the AST_EXT_MATCHCID_OFF (0) or AST_EXT_MATCHCID_ON (1) behaviors, and adds AST_EXT_MATCHCID_ANY (2) which allows matching to both exten->matchcid=0 and exten->matchcid=1 cases.  For example, all exten's with the same exten (number) can be deleted with ANY, but when looking for a dialplan match it must match to matchcid, and if set to ON, further match cidmatch.
> 
> The logic to match extensions has been reworked to implement these three types, and then matchcid values updated to use the enum for clarity.
> 
> The most significant change is in ast_context_remove_extension_callerid2() where after removing extensions from the hashtable, the deletion would destroy the extensions.  This was deleting exten's that didn't have matchcid set but matched a null cidmatch, where they hadn't been removed from the hashtable.
> 
> - (!callerid || !matchcallerid || (matchcallerid && !strcmp(peer->cidmatch, callerid))) &&
> + (!callerid || (!matchcallerid && !peer->matchcid) || (matchcallerid && peer->matchcid && !strcmp(peer->cidmatch, callerid))) &&
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/pbx.h 400531 
>   /branches/1.8/main/pbx.c 400531 
> 
> Diff: https://reviewboard.asterisk.org/r/2930/diff/
> 
> 
> Testing
> -------
> 
> This patch has been tested with the testsuite additions dialplan_reload and pbx/callerid_match (posted separately) to insure that the crash case has been resolved without any other unwanted changes in behavior.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131021/89351906/attachment-0001.html>