----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
