[asterisk-dev] [Code Review] 3650: pbx_config: Add manager command equivalents to 'dialplan add extension' and 'dialplan remove extension' CLI commands

[Mark Michelson]

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



/trunk/pbx/pbx_config.c
<https://reviewboard.asterisk.org/r/3650/#comment22565>

    I recommend use of a <literal> tag instead of the single quotes around the word hint.



/trunk/pbx/pbx_config.c
<https://reviewboard.asterisk.org/r/3650/#comment22566>

    The two error messages here are more-or-less the same, and for good reason. The problem in both cases is that the user gave an invalid priority number.
    
    You could combine these two sections into:
    
    } else if (sscanf(priority, "%30d", &ipriority != 1) || ipriority <= 0) {
        /* Send error message and return 0 */
    }
    
    The other option is to keep the code separated but give more specific error messages: One indicating a non-numeric priority was specified, and one indicating that a non-positive (if that's the correct word) priority was specified.
    
    I'm cool with either change.



/trunk/pbx/pbx_config.c
<https://reviewboard.asterisk.org/r/3650/#comment22567>

    I don't understand the comment about not substituting S_OR here. I would suspect that
    
    S_OR(cidmatch, ipriority ? "" : NULL)
    
    would do the same thing.



/trunk/pbx/pbx_config.c
<https://reviewboard.asterisk.org/r/3650/#comment22568>

    Make sure the priority evaluates to a positive value here.



/trunk/pbx/pbx_config.c
<https://reviewboard.asterisk.org/r/3650/#comment22569>

    Since you're already finding or creating an ast_context structure here. You may as well use the non-NULL return and call ast_add_extension2() instead of ast_add_extension(). That will save an extra hashtab lookup of the context.


- Mark Michelson


On June 26, 2014, 9:12 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3650/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 9:12 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds 'DialplanAddExtension' and 'DialplanRemoveExtension' manager commands that work in mostly the same way as their CLI command equivalents. The following header arguments are used for each:
> 
> Action: DialplanAddExtension
> Context - which context should be used
> Extension - name of the extension being created, if '/' is included, the portion after the '/' is a CID match for that extension.
> Priority - priority being added
> Application - name of the application to be used at this priority
> ApplicationData - not required (if not included results in no args), forms the arguments to the application
> Replace - not required (if not included, same as 'no'). If set to a truth value, replace existing extensions/priorities rather than failing if one exists where we are adding already.
> 
> Action: DialplanRemoveExtension
> Context - which context is being removed from
> Extension - Which extension is being removed or having a priority removed from, if '/' is included, the portion after the '/' is a CID match for that extension.
> Priority - not required, if included then just a single priority is removed from the extension instead of the whole extension.
> 
> A change to the pbx extension adding code was necessary in order for ast_add_extension to report an error when attempting to add an extension without replacing it when it already exists.
> The particular section in question previously had some developer comments questioning why the return values were what they were in the first place. I didn't observe any problematic behavior occuring as a result of the change, but it is in pbx.c, so I guess it could end up being a bit of a minefield.
> 
> 
> Diffs
> -----
> 
>   /trunk/pbx/pbx_config.c 416234 
>   /trunk/main/pbx.c 416234 
>   /trunk/CHANGES 416234 
> 
> Diff: https://reviewboard.asterisk.org/r/3650/diff/
> 
> 
> Testing
> -------
> 
> Tested add extension with/without appdata
> Tested add extension with/without '/' in extension and made sure the rest of the field was used as a CID value and that it worked the same as the CLI command equivalent
> Tested remove extension with/without priority
> Tested remove extension with/without '/' in extension and made sure that if CID was included that it deleted the CID including extension.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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