[asterisk-dev] PHPARI app_dial re-implementation
Hi All, I've managed to re-implement the basic functionality of app_dial using ARI and PHPARI. I've tested it and it supports handling of multiple calls at the same time. Having said that, I would highly appreciate some feedback in regards to the methodology, or if anybody can see something I can't. You can find the information here: http://www.phpari.org/documentation/app_dial-re-implemented-using-phpari/ And soon to be added to the 0.3 release of phpari. Regards, Nir S -- _ -- 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
Re: [asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
On Dec. 2, 2014, 9:30 a.m., opticron wrote: /branches/11/include/asterisk/module.h, lines 272-278 https://reviewboard.asterisk.org/r/4141/diff/1/?file=68704#file68704line272 These could use a bit of documentation. I've documented the macro's instead. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/#review13861 --- On Nov. 2, 2014, 2:13 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- (Updated Nov. 2, 2014, 2:13 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description --- This change includes an ABI change with compatibility stubs for 11, 12 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs - /branches/11/main/loader.c 426830 /branches/11/include/asterisk/module.h 426830 /branches/11/bridges/bridge_builtin_features.c 426830 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- 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
Re: [asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- (Updated Feb. 5, 2015, 3:36 a.m.) Review request for Asterisk Developers. Changes --- * Make all 3 functions always accept the REF_DEBUG parameters. They are not called enough to be worth the optimization by removing the unused parameters. * Updated patch to show actual code for 11. 13/trunk are different in bridge_builtin_features.c * Switched list iteration to normal (non _SAFE) as no elements are added/removed. * Address findings Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description (updated) --- This change includes an ABI change with compatibility stubs for 11 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs (updated) - /branches/11/main/loader.c 429406 /branches/11/include/asterisk/module.h 429406 /branches/11/bridges/bridge_builtin_features.c 429406 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- 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
Re: [asterisk-dev] [Code Review] 4405: Fix ast_odbc_find_table function in res_odbc
On Feb. 4, 2015, 8:24 p.m., Mark Michelson wrote: I don't see anything wrong in this patch, but I'm a bit confused about what this is fixing and how this is fixing it. Can you go into a bit more detail about why this fixes the problem you were seeing? wdoekes's explanation is better than mine. - ibercom --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4405/#review14411 --- On Feb. 4, 2015, 6:21 p.m., ibercom wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4405/ --- (Updated Feb. 4, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24742 https://issues.asterisk.org/jira/browse/ASTERISK-24742 Repository: Asterisk Description --- This patch fixes an anomalous behavior in ast_odbc_find_table function with Asterisk Realtime. - SELECT or INSERT operation generate a single command SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED in database engine before operation. - UPDATE operation generates two commands SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED in database engine before operation. The patch is trivial, just rearranges the call to ast_odbc_request_obj function. This isn't necessary when the table is cached. Diffs - branches/11/res/res_odbc.c 431571 Diff: https://reviewboard.asterisk.org/r/4405/diff/ Testing --- Log mysql usage. It is working for a week without problems. Thanks, ibercom -- _ -- 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
[asterisk-dev] [Code Review] 4407: cleanup various issues discovered during leak testing
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4407/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- A collection of small fixes to prevent Valgrind errors for your amusement and approval: 1) Config hooks: a) don't hold extra reference after creation of a hook, b) cleanup the container on shutdown 2) Util ast_join_delim: don't touch w[x] if x out of bounds of size 3) res_pjsip sync_task: prevent reference to sync task data struct if it gets freed after unlocked. Diffs - /branches/13/res/res_pjsip.c 431571 /branches/13/main/utils.c 431571 /branches/13/main/config.c 431571 Diff: https://reviewboard.asterisk.org/r/4407/diff/ Testing --- In each case Valgrind did not show the same error again after the patch was added. Thanks, Scott Griepentrog -- _ -- 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
Re: [asterisk-dev] [Code Review] 4399: HTTP: Stop accepting requests on final system shutdown.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4399/ --- (Updated Feb. 5, 2015, 3:58 p.m.) Review request for Asterisk Developers. Changes --- Addresses Mark's comments and pulled some shutdown code out of channel.c. Bugs: ASTERISK-24752 https://issues.asterisk.org/jira/browse/ASTERISK-24752 Repository: Asterisk Description (updated) --- There are three CLI commands to stop and restart Asterisk each. 1) core stop/restart now - Hangup all calls and stop or restart Asterisk. New channels are prevented while the shutdown request is pending. 2) core stop/restart gracefully - Stop or restart Asterisk when there are no calls remaining in the system. New channels are prevented while the shutdown request is pending. 3) core stop/restart when convenient - Stop or restart Asterisk when there are no calls in the system. New calls are not prevented while the shutdown request is pending. ARI has made stopping/restarting Asterisk more problematic. While a shutdown request is pending it is desirable to continue to process ARI HTTP requests for current calls. To handle the current calls while a shutdown request is pending, a new committed to shutdown phase is needed so ARI applications can deal with the calls until the system is fully committed to shutdown. * Added a new shutdown committed phase so ARI applications can deal with calls until the final committed to shutdown phase is reached. * Made refuse new HTTP requests when the system has reached the final system shutdown phase. Starting anything while the system is actively releasing resources and unloading modules is not a good thing. * Split the bridging framework shutdown to not cleanup the global bridging containers when shutting down in a hurry. This is similar to how other modules prevent crashes on rapid system shutdown. * Moved ast_begin_shutdown(), ast_cancel_shutdown(), and ast_shutting_down(). You should not have to include channel.h just to access these system functions. Diffs (updated) - /branches/13/res/res_pjsip_pubsub.c 431574 /branches/13/res/res_pjsip/pjsip_options.c 431574 /branches/13/main/http.c 431574 /branches/13/main/channel.c 431574 /branches/13/main/bridge.c 431574 /branches/13/main/asterisk.c 431574 /branches/13/include/asterisk/channel.h 431574 /branches/13/include/asterisk.h 431574 /branches/13/channels/chan_sip.c 431574 /branches/13/apps/app_confbridge.c 431574 Diff: https://reviewboard.asterisk.org/r/4399/diff/ Testing --- Extended the final shutdown phase sleep so I could send a HTTP request while in the final shutdown phase. The HTTP request was not refused while the shutdown request was pending and refused after the final shutdown phase was reached. Thanks, rmudgett -- _ -- 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
Re: [asterisk-dev] [Code Review] 4399: HTTP: Stop accepting requests on final system shutdown.
On Feb. 4, 2015, 2:17 p.m., Mark Michelson wrote: While a shutdown request is pending it is desirable to continue to process ARI HTTP requests for current calls I'm curious what the justification behind this is. I would expect that for a now or graceful shutdown, you would want similar behavior as for calls. That is, you'd want to stop accepting HTTP requests in order to allow for the shutdown to continue. For the when convenient shutdown, then I'd expect to continue taking HTTP requests until the final shutdown, like you've done here. Existing calls in a gracefull shutdown should be able to continue in an IVR by sending DTMF digits and having a stasis application play recordings back or the application could send the call to voicemail with a prompt that representatives are not available. Transfers obviously won't work because new channels cannot be created at this time but otherwise things should still function until the call hangs up. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4399/#review14410 --- On Feb. 3, 2015, 2:10 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4399/ --- (Updated Feb. 3, 2015, 2:10 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24752 https://issues.asterisk.org/jira/browse/ASTERISK-24752 Repository: Asterisk Description --- There are three CLI commands to stop and restart Asterisk each. 1) core stop/restart now - Hangup all calls and stop or restart Asterisk. 2) core stop/restart gracefully - Stop or restart Asterisk when there are no calls in the system. New channels are prevented while the shutdown request is pending. 3) core stop/restart when convenient - Stop or restart Asterisk when there are no calls in the system. New calls are not prevented while the shutdown request is pending. ARI has made stopping/restarting Asterisk more problematic. While a shutdown request is pending it is desirable to continue to process ARI HTTP requests for current calls. To handle the current calls while a shutdown request is pending, a new committed to shutdown phase is needed so ARI applications can deal with the calls until the system is fully committed to shutdown. * Added a new shutdown committed phase so ARI applications can deal with calls until the final committed to shutdown phase is reached. * Made refuse new HTTP requests when the system has reached the final system shutdown phase. * Split the bridging framework shutdown to not cleanup the global bridging containers when shutting down in a hurry. This is similar to how other modules prevent crashes on rapid system shutdown. * Moved prototypes for ast_begin_shutdown(), ast_cancel_shutdown(), and ast_shutting_down() to asterisk.h. You should not have to include channel.h just to access these system functions. Diffs - /branches/13/main/http.c 431537 /branches/13/main/bridge.c 431537 /branches/13/main/asterisk.c 431537 /branches/13/include/asterisk/channel.h 431537 /branches/13/include/asterisk.h 431537 Diff: https://reviewboard.asterisk.org/r/4399/diff/ Testing --- Extended the final shutdown phase sleep so I could send a HTTP request while in the final shutdown phase. The HTTP request was not refused while the shutdown request was pending and refused after the final shutdown phase was reached. Thanks, rmudgett -- _ -- 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
Re: [asterisk-dev] [Code Review] 4407: cleanup various issues discovered during leak testing
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4407/#review14419 --- Ship it! If you haven't already I'd also run any relevant unit and testsuite tests against these changes. - Kevin Harwell On Feb. 5, 2015, 4:25 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4407/ --- (Updated Feb. 5, 2015, 4:25 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- A collection of small fixes to prevent Valgrind errors for your amusement and approval: 1) Config hooks: a) don't hold extra reference after creation of a hook, b) cleanup the container on shutdown 2) Util ast_join_delim: don't touch w[x] if x out of bounds of size 3) res_pjsip sync_task: prevent reference to sync task data struct if it gets freed after unlocked. Diffs - /branches/13/res/res_pjsip.c 431571 /branches/13/main/utils.c 431571 /branches/13/main/config.c 431571 Diff: https://reviewboard.asterisk.org/r/4407/diff/ Testing --- In each case Valgrind did not show the same error again after the patch was added. Thanks, Scott Griepentrog -- _ -- 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
Re: [asterisk-dev] [Code Review] 4407: cleanup various issues discovered during leak testing
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4407/#review14420 --- 2) Util ast_join_delim: don't touch w[x] if x out of bounds of size Should be: 2) Utils.c ast_join_delim: don't touch w[x] if x out of bounds of size /branches/13/main/config.c https://reviewboard.asterisk.org/r/4407/#comment24938 Should set cfg_hooks = NULL after cleanup. - rmudgett On Feb. 5, 2015, 4:25 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4407/ --- (Updated Feb. 5, 2015, 4:25 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- A collection of small fixes to prevent Valgrind errors for your amusement and approval: 1) Config hooks: a) don't hold extra reference after creation of a hook, b) cleanup the container on shutdown 2) Util ast_join_delim: don't touch w[x] if x out of bounds of size 3) res_pjsip sync_task: prevent reference to sync task data struct if it gets freed after unlocked. Diffs - /branches/13/res/res_pjsip.c 431571 /branches/13/main/utils.c 431571 /branches/13/main/config.c 431571 Diff: https://reviewboard.asterisk.org/r/4407/diff/ Testing --- In each case Valgrind did not show the same error again after the patch was added. Thanks, Scott Griepentrog -- _ -- 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
[asterisk-dev] [Code Review] 4408: Testsuite: Add external bridging tests for Stasis (two channel) interactions
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4408/ --- Review request for Asterisk Developers and Mark Michelson. Bugs: ASTERISK-24611 https://issues.asterisk.org/jira/browse/ASTERISK-24611 Repository: testsuite Description --- This adds external bridging tests for Stasis (two channel) interactions as defined on the StasisStart/StasisEnd Test Plan (tests 2.5, 2.6, 2.7, and 2.8) at: https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=30279826#StasisStart/StasisEndTestplan-ExternalBridging This also renames (move to sub directory) the test 'tests/rest_api/external_interaction/ami_bridge/stasis_app/' to 'tests/rest_api/external_interaction/ami_bridge/stasis_app/non_stasis_app/'. NOTE: The files for the renamed test don't appear just because of how things work. Diffs - /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/tests.yaml 6377 /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/two_channel_same_stasis_app/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/two_channel_same_stasis_app/configs/ast1/extensions.conf PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/two_channel_different_stasis_app/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/two_channel_different_stasis_app/configs/ast1/extensions.conf PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/tests.yaml 6377 /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/two_channel_same_stasis_app/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/two_channel_same_stasis_app/configs/ast1/extensions.conf PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/two_channel_different_stasis_app/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/two_channel_different_stasis_app/configs/ast1/extensions.conf PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/tests.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/test-config.yaml 6377 /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_app/configs/ast1/extensions.conf 6377 Diff: https://reviewboard.asterisk.org/r/4408/diff/ Testing --- * Executed each test in a loop of 100 iterations with no failures. * Reviewed logs to ensure the tests were executing as expected. Thanks, jbigelow -- _ -- 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
Re: [asterisk-dev] [Code Review] 4405: Fix ast_odbc_find_table function in res_odbc
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4405/#review14416 --- Ship it! Ship It! - Mark Michelson On Feb. 5, 2015, 3:35 p.m., ibercom wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4405/ --- (Updated Feb. 5, 2015, 3:35 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24742 https://issues.asterisk.org/jira/browse/ASTERISK-24742 Repository: Asterisk Description --- This patch fixes an anomalous behavior in ast_odbc_find_table function with Asterisk Realtime. - SELECT or INSERT operation generate a single command SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED in database engine before operation. - UPDATE operation generates two commands SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED in database engine before operation. The patch is trivial, just rearranges the call to ast_odbc_request_obj function. This isn't necessary when the table is cached. Diffs - branches/11/res/res_odbc.c 431571 Diff: https://reviewboard.asterisk.org/r/4405/diff/ Testing --- Log mysql usage. It is working for a week without problems. Thanks, ibercom -- _ -- 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
Re: [asterisk-dev] [Code Review] 4379: Example configuration scenario - Super Awesome Company: Phase 1 - Patch 1
On Jan. 29, 2015, 10:23 p.m., Mark Michelson wrote: /branches/13/configs/examples/super_awesome_company/pjsip.conf, line 41 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71114#file71114line41 I'm curious why you elected to use MAC addresses as the endpoint names. I'd personally find things a lot easier to configure/maintain if the SIP endpoint/aor/auth name is the same as the voicemail box number is the same as the extension number, etc. This also means that if Lindsey does some crazy extreme stunt that smashes her phone, then when she replaces it with a new one, you're going to have to change config values everywhere to have the new MAC address of the phone. Matt Jordan wrote: Hm. I think that's usually one of those best practices. You generally don't want the auth user to be easily guessed. Of course, we could split the concept of the endpoint name from the auth user, which would then allow the endpoints to be named 107 (for example) and the auth user to be her MAC address. Joshua Colp wrote: I think in practice this would just cause problems. Not all devices allow those two things to be separate. It's annoying. Mark Michelson wrote: SAC uses Digium phones, and Digium phones allow separate user and authuser to be specified. Joshua Colp wrote: Your statement is true but it would be nice if we could err on the side of not falling into a trap of doing fundamental stuff which isn't applicable to the wide world. I used MAC addresses as that is what we use as an example in our security best practices document: http://svnview.digium.com/svn/asterisk/trunk/README-SERIOUSLY.bestpractices.txt?view=markup Perhaps this is a moot point. SAC's Asterisk system is behind NAT and firewall, so we could change the spec to specify that IT has locked down traffic between Asterisk and the public internet to only allow inbound traffic from the ITSP addresses. Or, on Asterisk we can use ACL's to limit traffic allowed to the internal network and ITSP addresses. With either of those approaches we should be able to use the less secure extension numbered auth users. What would be the issues either of these approaches other than an attacker on the internal network? - rnewton --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/#review14377 --- On Jan. 27, 2015, 7:15 p.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/ --- (Updated Jan. 27, 2015, 7:15 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- One of things discussed at the last AstriDevCon was better documentation (for everything!), but in particular, we mentioned needing some example configurations that pertain to a real-world scenario. That is, as opposed to the current sample files which are sort of all over the place at this point. This patch proposes a basic and minimal configuration of Asterisk to satisfy the requirements for the first phase of Super Awesome Company's implementation of Asterisk. I will submit four separate patches for the first phase, so that we don't have to review the entire thing all at once. This review is for the first patch. Who is Super Awesome Company? See https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company For the first patch, I am attempting to satisfy the below requirements. The patch does not include a new make target, as I believe Matt Jordan offered to handle that. SAC requires: * PJSIP connectivity for all employee desk phones. * The ability for employees to call one another inside of the office. * Voicemail boxes for each of the employees. Basic configuration We want SAC to have a clean system. That means: * No 'autoload' in modules.conf. Explicitly load a basic configuration. If SAC doesn't need the module, don't load it. * Every module loaded should have a configuration file that is appropriate for it. This includes all the 'core' things that need configuration. pjsip.conf * A PJSIP configuration for their desk phones. Assume every endpoint that is a phone has: * A voicemail mailbox that they can subscribe to * A hint for their device * Note that the PJSIP configuration should adhere to best practices. That means MAC addresses for device names, etc. extensions.conf * A safe dialplan for intra-company communication. This should be templated out so that it is trivial to add additional devices (use pattern matching/pattern matching hints, etc.) *
Re: [asterisk-dev] [Code Review] 4365: Adding AMQP backend for CDR and CEL
On Feb. 4, 2015, 8:21 a.m., Joshua Colp wrote: /branches/13/include/asterisk/cel.h, lines 37-38 https://reviewboard.asterisk.org/r/4365/diff/3/?file=71072#file71072line37 Why were these added here? Both are used in this file. If you don't happen to #include them prior to cel.h in your .c file, it fails to compile. On Feb. 4, 2015, 8:21 a.m., Joshua Colp wrote: /branches/13/res/amqp/config.c, lines 228-240 https://reviewboard.asterisk.org/r/4365/diff/3/?file=71076#file71076line228 This should be done in the pre_apply_config callback so the configuration is completely validated before doing the atomic swap. As well - should connection validation failure cause the config to not get applied in that case? should connection validation failure cause the config to not get applied in that case? Yup. Fixed. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/#review14407 --- On Jan. 26, 2015, 9:16 a.m., David Lee wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- (Updated Jan. 26, 2015, 9:16 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs - /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 431095 /branches/13/makeopts.in 431095 /branches/13/include/asterisk/cel.h 431095 /branches/13/include/asterisk/autoconfig.h.in 431095 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 431095 /branches/13/configure.ac 431095 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 431095 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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
Re: [asterisk-dev] [Code Review] 4365: Adding AMQP backend for CDR and CEL
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- (Updated Feb. 5, 2015, 7:13 p.m.) Review request for Asterisk Developers. Changes --- Addressed review feedback. * Used pre_apply_config * Fixed a doc typo Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs (updated) - /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 431095 /branches/13/makeopts.in 431095 /branches/13/include/asterisk/cel.h 431095 /branches/13/include/asterisk/autoconfig.h.in 431095 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 431095 /branches/13/configure.ac 431095 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 431095 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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
Re: [asterisk-dev] [Code Review] 4379: Example configuration scenario - Super Awesome Company: Phase 1 - Patch 1
On Jan. 27, 2015, 8:34 p.m., Matt Jordan wrote: /branches/13/configs/examples/super_awesome_company/pjsip.conf, lines 24-30 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71114#file71114line24 I'd rename these to: auth-basic aor-basic Since they are supposed to generally accompany 'endpoint-basic' My idea was rather to have the name of each object reflect the unique configuration of that object. If I go to add new endpoints, the generic titles of basic, advanced, etc won't make it quick to remember which templates I want to use for my new endpoint. What I may do is change the endpoint object title to better reflect its usage. 'endpoint-basic' isn't helpful. Maybe endpoint-internal-d70. Let me know if that makes sense. On Jan. 27, 2015, 8:34 p.m., Matt Jordan wrote: /branches/13/configs/examples/super_awesome_company/voicemail.conf, lines 9-23 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71115#file71115line9 All of them should have VoiceMail pins SAC provides an extension, 800, that lets internal employees directly dial their voicemail box. When internal SAC employees dial their voicemail box, they’re not prompted for their mailbox number or their PIN code. Voicemail may be accessed remotely by employees who dial 256-555-1234. When employees dial voicemail remotely, they must input both their mailbox number and their pin code. Aha. I read the first line, but missed the pin code mention on the second. I just remembered the 's' option for voicemailmain as well to have it ignore the pin. - rnewton --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/#review14320 --- On Jan. 27, 2015, 7:15 p.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/ --- (Updated Jan. 27, 2015, 7:15 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- One of things discussed at the last AstriDevCon was better documentation (for everything!), but in particular, we mentioned needing some example configurations that pertain to a real-world scenario. That is, as opposed to the current sample files which are sort of all over the place at this point. This patch proposes a basic and minimal configuration of Asterisk to satisfy the requirements for the first phase of Super Awesome Company's implementation of Asterisk. I will submit four separate patches for the first phase, so that we don't have to review the entire thing all at once. This review is for the first patch. Who is Super Awesome Company? See https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company For the first patch, I am attempting to satisfy the below requirements. The patch does not include a new make target, as I believe Matt Jordan offered to handle that. SAC requires: * PJSIP connectivity for all employee desk phones. * The ability for employees to call one another inside of the office. * Voicemail boxes for each of the employees. Basic configuration We want SAC to have a clean system. That means: * No 'autoload' in modules.conf. Explicitly load a basic configuration. If SAC doesn't need the module, don't load it. * Every module loaded should have a configuration file that is appropriate for it. This includes all the 'core' things that need configuration. pjsip.conf * A PJSIP configuration for their desk phones. Assume every endpoint that is a phone has: * A voicemail mailbox that they can subscribe to * A hint for their device * Note that the PJSIP configuration should adhere to best practices. That means MAC addresses for device names, etc. extensions.conf * A safe dialplan for intra-company communication. This should be templated out so that it is trivial to add additional devices (use pattern matching/pattern matching hints, etc.) * Receiving a Busy/Unavailable should result in going to VoiceMail * A user should be able to dial something and get to their VoiceMailMain without having to enter in their extension number * Note that mapping of MAC address endpoints to extension numbers should be done in some fashion that is easily extensible. voicemail.conf * Set up mailboxes for every person in SAC. Assign 'default' pins. Create reasonable basic settings. * Do not set up e-mail or pager addresses. REVIEW? Please, if possible look at this from a few angles: * Use the configuration, configure a couple phones and call between them. Leave voicemails and retrieve them. * Have I
Re: [asterisk-dev] [Code Review] 4379: Example configuration scenario - Super Awesome Company: Phase 1 - Patch 1
On Jan. 30, 2015, 1:13 p.m., Joshua Colp wrote: /branches/13/configs/examples/super_awesome_company/modules.conf, line 41 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71112#file71112line41 Add format_pcm. This covers ulaw and g722 as well. On installation it's wise to have these sounds installed as well. It was already on there. :) - rnewton --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/#review14391 --- On Jan. 27, 2015, 7:15 p.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/ --- (Updated Jan. 27, 2015, 7:15 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- One of things discussed at the last AstriDevCon was better documentation (for everything!), but in particular, we mentioned needing some example configurations that pertain to a real-world scenario. That is, as opposed to the current sample files which are sort of all over the place at this point. This patch proposes a basic and minimal configuration of Asterisk to satisfy the requirements for the first phase of Super Awesome Company's implementation of Asterisk. I will submit four separate patches for the first phase, so that we don't have to review the entire thing all at once. This review is for the first patch. Who is Super Awesome Company? See https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company For the first patch, I am attempting to satisfy the below requirements. The patch does not include a new make target, as I believe Matt Jordan offered to handle that. SAC requires: * PJSIP connectivity for all employee desk phones. * The ability for employees to call one another inside of the office. * Voicemail boxes for each of the employees. Basic configuration We want SAC to have a clean system. That means: * No 'autoload' in modules.conf. Explicitly load a basic configuration. If SAC doesn't need the module, don't load it. * Every module loaded should have a configuration file that is appropriate for it. This includes all the 'core' things that need configuration. pjsip.conf * A PJSIP configuration for their desk phones. Assume every endpoint that is a phone has: * A voicemail mailbox that they can subscribe to * A hint for their device * Note that the PJSIP configuration should adhere to best practices. That means MAC addresses for device names, etc. extensions.conf * A safe dialplan for intra-company communication. This should be templated out so that it is trivial to add additional devices (use pattern matching/pattern matching hints, etc.) * Receiving a Busy/Unavailable should result in going to VoiceMail * A user should be able to dial something and get to their VoiceMailMain without having to enter in their extension number * Note that mapping of MAC address endpoints to extension numbers should be done in some fashion that is easily extensible. voicemail.conf * Set up mailboxes for every person in SAC. Assign 'default' pins. Create reasonable basic settings. * Do not set up e-mail or pager addresses. REVIEW? Please, if possible look at this from a few angles: * Use the configuration, configure a couple phones and call between them. Leave voicemails and retrieve them. * Have I created any security issues? * Is my dialplan easy to understand? * Could anything be done more efficiently without making it over-complicated? * Have I over-complicated anything? * Are there any critical settings I'm missing from any of the files? A couple, more specific questions: * We have sample configs in /configs/samples; what directory do we want these configurations in? (I used /configs/examples for now, but I don't really like it) * We have the make target make samples for the current samples; what do we want for these new configs? Diffs - /branches/13/configs/examples/super_awesome_company/voicemail.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/pjsip.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/musiconhold.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/modules.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/logger.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/indications.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/extensions.conf PRE-CREATION /branches/13/configs/examples/super_awesome_company/asterisk.conf
Re: [asterisk-dev] [Code Review] 4405: Fix ast_odbc_find_table function in res_odbc
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4405/#review14414
---
Ship it!
LGTM.
branches/11/res/res_odbc.c
https://reviewboard.asterisk.org/r/4405/#comment24936
Remove this if(obj) while you're in the vicinity. It's always true.
@Mark: it's calling ast_odbc_request_obj and ast_odbc_release_obj, which is not
needed unless we are actually going use obj.
The effects are that ast_odbc_find_table() will call SQL_ATTR_TXN_ISOLATION
setup code, which has already been called anyway and which will be called
another time when it's actually used. Like this:
static int update_odbc(
...
tableptr = ast_odbc_find_table(database, table);
if (!(obj = ast_odbc_request_obj2(database, connected_flag))) {
- wdoekes
On Feb. 4, 2015, 6:21 p.m., ibercom wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4405/
---
(Updated Feb. 4, 2015, 6:21 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24742
https://issues.asterisk.org/jira/browse/ASTERISK-24742
Repository: Asterisk
Description
---
This patch fixes an anomalous behavior in ast_odbc_find_table function with
Asterisk Realtime.
- SELECT or INSERT operation generate a single command SET SESSION
TRANSACTION ISOLATION LEVEL READ COMMITTED in database engine before
operation.
- UPDATE operation generates two commands SET SESSION TRANSACTION ISOLATION
LEVEL READ COMMITTED in database engine before operation.
The patch is trivial, just rearranges the call to ast_odbc_request_obj
function. This isn't necessary when the table is cached.
Diffs
-
branches/11/res/res_odbc.c 431571
Diff: https://reviewboard.asterisk.org/r/4405/diff/
Testing
---
Log mysql usage.
It is working for a week without problems.
Thanks,
ibercom
--
_
-- 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
Re: [asterisk-dev] [Code Review] 4379: Example configuration scenario - Super Awesome Company: Phase 1 - Patch 1
On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/README, line 2 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71107#file71107line2 maybe shorten directory name to awesome, or sae? Or, another option would be to use Acme, Inc. I think awesome works. On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/asterisk.conf, line 1 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71108#file71108line1 My thoughts of this file is just to symbiotic link the current asterisk.conf file. Since, you are'nt really doing anything here. If someone is using this example, there shouldn't be a current asterisk.conf file. The example asterisk.conf has trimmed documentation down to the most basic of what you might want to change for a simple setup. On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/asterisk.conf, lines 28-36 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71108#file71108line28 This seems to be dead code, as such just remove? I think we should leave it there to emphasize the importance of it. This example is both a template and an example, so they may want to uncomment these lines. It is hard to strike a balance between removing everything they might need to uncomment and leaving everything in (ending up back with the previous .conf.sample files). Should we explain more in the comments when we would uncomment it? On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/extensions.conf, lines 6-20 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71109#file71109line6 While this is good practices, in the real world using MAC for actual extensions is bulky and complicated. I would suggest using actually 101-115 as extension numbers but using mac for the authenticate portion of your sip.conf files. Maybe we can change this. See conversation in Mark's comment on same topic. On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/extensions.conf, line 42 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71109#file71109line42 I'd also prefer to see the first time always a naked NoOp() exten = 8000,1,NoOp() same = n,Verbose(1, blah) this way, when jumping into new contexts, you know you'll always hit a naked noop. Can you explain the usefulness of this more? On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/extensions.conf, line 50 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71109#file71109line50 using ._ is not recommend practices and will generate warnings in asterisk. Change to _X. Oops, thought I had _X., thanks. On Jan. 30, 2015, 7:51 p.m., Paul Belanger wrote: /branches/13/configs/examples/super_awesome_company/extensions.conf, lines 53-66 https://reviewboard.asterisk.org/r/4379/diff/1/?file=71109#file71109line53 I disagree with this comments. Moving subroutines into there own contexts allow you to reuse code between applications. However, this example does not do this, it could be possible to create generate subroutines in asterisk samples to share across my examples. I ended up rewriting this whole section for a few reasons. - rnewton --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/#review14401 --- On Jan. 27, 2015, 7:15 p.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4379/ --- (Updated Jan. 27, 2015, 7:15 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- One of things discussed at the last AstriDevCon was better documentation (for everything!), but in particular, we mentioned needing some example configurations that pertain to a real-world scenario. That is, as opposed to the current sample files which are sort of all over the place at this point. This patch proposes a basic and minimal configuration of Asterisk to satisfy the requirements for the first phase of Super Awesome Company's implementation of Asterisk. I will submit four separate patches for the first phase, so that we don't have to review the entire thing all at once. This review is for the first patch. Who is Super Awesome Company? See https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company For the first patch, I am attempting to
