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

[Jonathan Rose]

> On June 26, 2014, 6:18 p.m., Mark Michelson wrote:
> > /trunk/pbx/pbx_config.c, lines 525-526
> > <https://reviewboard.asterisk.org/r/3650/diff/3/?file=61090#file61090line525>
> >
> >     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.

Well, both the comment and the check came from the CLI command. I think you'd have to ask Tilghman for the justification if I'm following the svn blame log correctly. I'm a little nervous to change this to S_OR when the comment explicitly said what it said.


> On June 26, 2014, 6:18 p.m., Mark Michelson wrote:
> > /trunk/pbx/pbx_config.c, lines 1199-1204
> > <https://reviewboard.asterisk.org/r/3650/diff/3/?file=61090#file61090line1199>
> >
> >     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.

I went ahead and did this, but I should note that this requires locking the contexts list in order to make it safe since the context could potentially already be blown away by something else by the time ast_context_find_or_create returns.


- Jonathan


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


On June 26, 2014, 4: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, 4: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/20140627/fbdc3a43/attachment.html>