Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
On Jan. 29, 2015, 8:22 p.m., George Joseph wrote: If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible. gareth wrote: I ran the test apps/queue/set_penalty which makes use of ami.command and it failed. However this appears to be a bug with starpy, it is treading the \r\n as the end of message. Changing it to output just \n results in a successful test. Breaking an existing client library isn't ideal, but the correct delimiter for command output is --END COMMAND--\r\n\r\n, not \r\n\r\n. If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs. I'd rather see the command output converted to headers: Response: Success ActionID: actionid Output: line 1 of cli output Output: more Output headers per line of cli output This would of course break existing clients, but would get rid of the exception to the AMI protocol. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/#review14384 --- On Jan. 29, 2015, 7:25 p.m., gareth wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/ --- (Updated Jan. 29, 2015, 7:25 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24730 https://issues.asterisk.org/jira/browse/ASTERISK-24730 Repository: Asterisk Description --- This patch adds a blank line between the headers and the output in the Command action response. The reason for this change is to make it easier to determine where the headers end and the output from the command starts. Currently, to parse a response to a Command action, one has to: 1. Read one line, if line is Response: Error, parse the remaining as a standard AMI response and stop. 2. Read one more line - or two if you used an ActionID - those lines are the headers. 3. Then read everything up to --END COMMAND--\r\n\r\n. That could be changed to: 1. Read standard AMI response. 2. If Response: Follows, then read everything up to --END COMMAND--\r\n\r\n. The AMI version has been increased to 2.8.0 as this is a protocol change and so that clients detect the new behavior. Adding a blank line should not cause older parsers to fail as they have to read everything up to --END COMMAND--\r\n\r\n anyway. Adding a blank line will also not cause the AMI to HTML/XML encoder to fail to separate the headers from the output as it checks for the presence of a : character, which a blank line does not contain. Diffs - /trunk/main/manager.c 431113 /trunk/include/asterisk/manager.h 431113 /trunk/CHANGES 431113 Diff: https://reviewboard.asterisk.org/r/4391/diff/ Testing --- Connected to manager, issued 'core show uptime' command and verified that there was a blank line between the headers and output. Thanks, gareth -- _ -- 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] 4489: res_pjsip: allow configuration of endpoint identifier query order
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4489/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24840 https://issues.asterisk.org/jira/browse/ASTERISK-24840 Repository: Asterisk Description --- Make sure that no matter what order the endpoint identifier modules were loaded, priority is given based on the ones specified in the new global 'endpoint_identifier_order' option. The original patch for this issue had to be reverted due to some problems it created (original review: https://reviewboard.asterisk.org/r/4455/). However, those problems have now been alleviated with the current patch on this review. Instead of having the associated endpoint identifier name directly contained on the identifier object (part of the externally exposed struct definition) the name is now passed in as part of the register function. The old register function has been left for backwards compatibility or for any modules that don't need to register with a name. Diffs - branches/13/res/res_pjsip_endpoint_identifier_user.c 432869 branches/13/res/res_pjsip_endpoint_identifier_ip.c 432869 branches/13/res/res_pjsip_endpoint_identifier_anonymous.c 432869 branches/13/res/res_pjsip/config_global.c 432869 branches/13/res/res_pjsip.c 432869 branches/13/include/asterisk/res_pjsip.h 432869 branches/13/contrib/ast-db-manage/config/versions/45e3f47c6c44_add_pjsip_endpoint_identifier_order.py 432869 branches/13/configs/samples/pjsip.conf.sample 432869 branches/13/CHANGES 432869 Diff: https://reviewboard.asterisk.org/r/4489/diff/ Testing --- Ran the testsuite test and it still passed. Modified the test by changing the order of the identifiers and it failed (as it should have). Thanks, Kevin Harwell -- _ -- 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] 4478: testsuite: Fix reference leaks and shutdown timeouts in more tests
On March 12, 2015, 2:11 p.m., Mark Michelson wrote: Regarding the remaining leak on the local optimize away test, if you do find that it is a leak in Asterisk, please file an issue for it. ASTERISK-24876 created. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4478/#review14662 --- On March 13, 2015, 1:30 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4478/ --- (Updated March 13, 2015, 1:30 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- * apps/page/page_baseline: hangup channels ignoring errors * apps/queues/position_priority_maxlen: hangup channels * callparking_retrieval: hangup parkee * channels/SIP/sip_tls_call: hangup the channel that gives the last DTMF event * channels/local/local_optimize_away: no longer has shutdown timeout, but still has some reference leaks. This might be an Asterisk leak. * rest_api/events/user/multi: requires chan_pjsip. The missing of dependency was causing Bamboo failures. * rest_api/external_interaction/ami_bridge/stasis_bridge/non_stasis_app: delete the bridge * rest_api/request-bodies: delete the bridges Diffs - /asterisk/trunk/tests/rest_api/request-bodies/run-test 6522 /asterisk/trunk/tests/rest_api/external_interaction/ami_bridge/stasis_bridge/non_stasis_app/test-config.yaml 6522 /asterisk/trunk/tests/rest_api/events/user/multi/test-config.yaml 6522 /asterisk/trunk/tests/channels/local/local_optimize_away/test-config.yaml 6522 /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test 6522 /asterisk/trunk/tests/callparking_retrieval/run-test 6522 /asterisk/trunk/tests/apps/queues/position_priority_maxlen/run-test 6522 /asterisk/trunk/tests/apps/page/page_baseline/run-test 6522 Diff: https://reviewboard.asterisk.org/r/4478/diff/ Testing --- Of the modified tests only channels/local/local_optimize_away still fails for leaks. 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] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
On March 12, 2015, 3:41 p.m., Matt Jordan wrote: 1. For this to go into Asterisk 13, tests will need to be provided that cover the new parameter. (Really, those tests should be written regardless) 2. The CHANGES file will need to get updated with the new option. rmudgett wrote: Actually I'd prefer that the rpid_immediate option not exist at all and the code it controls to just be removed. Sending connected line updates back to the caller _before_ getting connected doesn't really make sense. This is what the REDIRECTING information is supposed to be doing. gareth wrote: I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered? That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history. If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used? OK, that makes sense but the code that rpid_immediate controls doesn't. As described in the review description, that code immediately sends redundant 180 Ringing or 183 Progress messages at best and at worst lies that the other end is ringing with inaccurate connected line information. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 13, 2015, 12:13 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 12:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end even gets the call. * Many phones tend to grab the first connected line information and refuse to update the display if it changes. The first information is not likely to be correct if the call goes to an endpoint not under the control of the first Asterisk box. When connected line first went into Asterisk in v1.8, chan_sip received an undocumented option rpid_immediate that defaults to disabled. When enabled, the option immediately passes connected line update information to the caller in 180 Ringing or 183 Progress messages as described above. * Added rpid_immediate option to prevent unnecessary 180 Ringing or 183 Progress messages. The default is no to disable sending the unnecessary messages. Diffs - /branches/13/res/res_pjsip/pjsip_configuration.c 432895 /branches/13/res/res_pjsip.c 432895 /branches/13/include/asterisk/res_pjsip.h 432895 /branches/13/configs/samples/pjsip.conf.sample 432895 /branches/13/channels/chan_pjsip.c 432895 Diff: https://reviewboard.asterisk.org/r/4473/diff/ Testing --- * Ran the tests/channels/pjsip testsuite tests. They still pass. * Made a call chain as follows: 100 - * - * - * - 200. With the patch there are no unnecessary messages. Without the patch there were several 180 Ringing messages sent back to the caller. 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] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 12:13 p.m.) Review request for Asterisk Developers. Changes --- Updated patch with https://reviewboard.asterisk.org/r/4472/ committed. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description (updated) --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end even gets the call. * Many phones tend to grab the first connected line information and refuse to update the display if it changes. The first information is not likely to be correct if the call goes to an endpoint not under the control of the first Asterisk box. When connected line first went into Asterisk in v1.8, chan_sip received an undocumented option rpid_immediate that defaults to disabled. When enabled, the option immediately passes connected line update information to the caller in 180 Ringing or 183 Progress messages as described above. * Added rpid_immediate option to prevent unnecessary 180 Ringing or 183 Progress messages. The default is no to disable sending the unnecessary messages. Diffs (updated) - /branches/13/res/res_pjsip/pjsip_configuration.c 432895 /branches/13/res/res_pjsip.c 432895 /branches/13/include/asterisk/res_pjsip.h 432895 /branches/13/configs/samples/pjsip.conf.sample 432895 /branches/13/channels/chan_pjsip.c 432895 Diff: https://reviewboard.asterisk.org/r/4473/diff/ Testing --- * Ran the tests/channels/pjsip testsuite tests. They still pass. * Made a call chain as follows: 100 - * - * - * - 200. With the patch there are no unnecessary messages. Without the patch there were several 180 Ringing messages sent back to the caller. 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] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
On March 12, 2015, 3:41 p.m., Matt Jordan wrote: 1. For this to go into Asterisk 13, tests will need to be provided that cover the new parameter. (Really, those tests should be written regardless) 2. The CHANGES file will need to get updated with the new option. rmudgett wrote: Actually I'd prefer that the rpid_immediate option not exist at all and the code it controls to just be removed. Sending connected line updates back to the caller _before_ getting connected doesn't really make sense. This is what the REDIRECTING information is supposed to be doing. gareth wrote: I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered? That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history. If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used? rmudgett wrote: OK, that makes sense but the code that rpid_immediate controls doesn't. As described in the review description, that code immediately sends redundant 180 Ringing or 183 Progress messages at best and at worst lies that the other end is ringing with inaccurate connected line information. I'm not sure what this setting is attempting to do then. The description of the new setting makes it sound like it controls sending connected line information prior to Answer, which fits gareth's interpretation. Whether or not it sends multiple 180/183 provisional responses does not seem to be implied in the description of the setting. If setting 'rpid_immediate' to 'yes' still results in extraneous 180/183 provisional responses, then I'm not sure this patch fixes the underlying issue. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 13, 2015, 12:13 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 12:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end even gets the call. * Many phones tend to grab the first connected line information and refuse to update the display if it changes. The first information is not likely to be correct if the call goes to an endpoint not under the control of the first Asterisk box. When connected line first went into Asterisk in v1.8, chan_sip received an undocumented option rpid_immediate that defaults to disabled. When enabled, the option immediately passes connected line update information to the caller in 180 Ringing or 183 Progress messages as described above. * Added rpid_immediate option to prevent unnecessary 180 Ringing or 183 Progress messages. The default is no to disable sending the unnecessary messages. Diffs - /branches/13/res/res_pjsip/pjsip_configuration.c 432895 /branches/13/res/res_pjsip.c 432895 /branches/13/include/asterisk/res_pjsip.h 432895 /branches/13/configs/samples/pjsip.conf.sample 432895 /branches/13/channels/chan_pjsip.c 432895 Diff: https://reviewboard.asterisk.org/r/4473/diff/ Testing --- * Ran the tests/channels/pjsip testsuite tests. They still pass. * Made a call chain as follows: 100 - * - * - * - 200. With the patch there are no unnecessary messages. Without the patch there were several 180 Ringing messages sent back to the caller. 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
[asterisk-dev] [Code Review] 4189: chan_sip: Simplify dialog/peer references, add REF_DEBUG passthrough of callers to sip_alloc and find_call.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4189/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- This does have a minor change to sip_ref_peer and dialog_ref - the error messages about trying to reference a NULL is removed. This message provided nothing useful. The changes to sip_alloc / find_call make it easier to trace REF_DEBUG logs for leaked dialogs. Note: I've posted the version of this patch for 13. In trunk the 'struct ast_callid *' type has been replaced with a typedef 'ast_callid', effecting the parameter logger_callid of sip_alloc. Diffs - /branches/13/channels/sip/include/sip.h 432806 /branches/13/channels/sip/include/dialog.h 432806 /branches/13/channels/chan_sip.c 432806 Diff: https://reviewboard.asterisk.org/r/4189/diff/ Testing --- Ran a few testsuite chan_sip tests. Verified that REF_DEBUG log shows caller of sip_alloc. 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] 4489: res_pjsip: allow configuration of endpoint identifier query order
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4489/#review14685 --- Ship it! Ship It! branches/13/include/asterisk/res_pjsip.h https://reviewboard.asterisk.org/r/4489/#comment25239 You may want to add a comment to the other function detailing that it will be placed at the front. branches/13/include/asterisk/res_pjsip.h https://reviewboard.asterisk.org/r/4489/#comment25240 Pedantic: with_name instead of by_name, but that's just me - Joshua Colp On March 13, 2015, 5:15 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4489/ --- (Updated March 13, 2015, 5:15 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24840 https://issues.asterisk.org/jira/browse/ASTERISK-24840 Repository: Asterisk Description --- Make sure that no matter what order the endpoint identifier modules were loaded, priority is given based on the ones specified in the new global 'endpoint_identifier_order' option. The original patch for this issue had to be reverted due to some problems it created (original review: https://reviewboard.asterisk.org/r/4455/). However, those problems have now been alleviated with the current patch on this review. Instead of having the associated endpoint identifier name directly contained on the identifier object (part of the externally exposed struct definition) the name is now passed in as part of the register function. The old register function has been left for backwards compatibility or for any modules that don't need to register with a name. Diffs - branches/13/res/res_pjsip_endpoint_identifier_user.c 432869 branches/13/res/res_pjsip_endpoint_identifier_ip.c 432869 branches/13/res/res_pjsip_endpoint_identifier_anonymous.c 432869 branches/13/res/res_pjsip/config_global.c 432869 branches/13/res/res_pjsip.c 432869 branches/13/include/asterisk/res_pjsip.h 432869 branches/13/contrib/ast-db-manage/config/versions/45e3f47c6c44_add_pjsip_endpoint_identifier_order.py 432869 branches/13/configs/samples/pjsip.conf.sample 432869 branches/13/CHANGES 432869 Diff: https://reviewboard.asterisk.org/r/4489/diff/ Testing --- Ran the testsuite test and it still passed. Modified the test by changing the order of the identifiers and it failed (as it should have). Thanks, Kevin Harwell -- _ -- 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] 4488: Super Awesome Company: Phase 1 - Patch 2 - Outside Connectivity!
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4488/#review14688 --- /branches/13/configs/basic-pbx/extensions.conf https://reviewboard.asterisk.org/r/4488/#comment25241 Since we already know what the numbers for these queues are going to be, you could probably go ahead and put the GOTO operations in there and stub them out. - Jonathan Rose On March 13, 2015, 9:32 a.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4488/ --- (Updated March 13, 2015, 9:32 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Howdy, here is another patch for the Super Awesome Company configuration. We are still in phase 1. The general requirements are posted on the wiki: https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company The specific requirements this patch meets are below: pjsip.conf * SIP ITSP configuration example and have place holders for the required authentication bits. ** Assume that Asterisk does not have a public IP address, and sits behind a NAT with its desk phones. * Have outbound registration to the SIP trunk, and an endpoint that represents the SIP trunk. * Inbound calls received from the SIP trunk should go into their own context. extensions.conf * Match the outbound dial request so that it can only dial US area codes. ** Don't let people dial 900 numbers, international numbers, or any other numbers that could result in a charge * Inbound calls from the SIP trunk should hit a basic Auto Attendant that prompts them for the extension to dial, after greeting them to SAC. * If an inbound call matches a DID that maps to a specific extension/device, dial that extension/device directly. Billing * Make sure CDRs output all calls that are from/to the SIP trunk. These should be logged to a CSV. * For intra-office calls, kill the CDRs. Additional Requirements Noted: * For outbound calls, each SAC employee’s 10-digit DID number is provided as their Caller ID. * 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. * 7, 10 and 10+1 digit dialing for local and long distance calls. * Internal dialing of otherwise inbound features, ** 1100 to reach the main IVR. * The IVR options possible without getting into Phase 2. Diffs - /branches/13/configs/basic-pbx/pjsip.conf 432866 /branches/13/configs/basic-pbx/modules.conf 432866 /branches/13/configs/basic-pbx/logger.conf 432866 /branches/13/configs/basic-pbx/extensions.conf 432866 Diff: https://reviewboard.asterisk.org/r/4488/diff/ Testing --- Setup with a Digium Cloud Services trunk and a few internal phones. Internal to Internal calls. Calls Internal to voicemail and other features. External to internal DID calls. External to internal feature calls. Basically tried to call as many ways as I could through all the various features. Everything seemed to work. Thanks, rnewton -- _ -- 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] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
On March 12, 2015, 3:41 p.m., Matt Jordan wrote: 1. For this to go into Asterisk 13, tests will need to be provided that cover the new parameter. (Really, those tests should be written regardless) 2. The CHANGES file will need to get updated with the new option. rmudgett wrote: Actually I'd prefer that the rpid_immediate option not exist at all and the code it controls to just be removed. Sending connected line updates back to the caller _before_ getting connected doesn't really make sense. This is what the REDIRECTING information is supposed to be doing. gareth wrote: I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered? That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history. If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used? rmudgett wrote: OK, that makes sense but the code that rpid_immediate controls doesn't. As described in the review description, that code immediately sends redundant 180 Ringing or 183 Progress messages at best and at worst lies that the other end is ringing with inaccurate connected line information. Matt Jordan wrote: I'm not sure what this setting is attempting to do then. The description of the new setting makes it sound like it controls sending connected line information prior to Answer, which fits gareth's interpretation. Whether or not it sends multiple 180/183 provisional responses does not seem to be implied in the description of the setting. If setting 'rpid_immediate' to 'yes' still results in extraneous 180/183 provisional responses, then I'm not sure this patch fixes the underlying issue. The option does exactly the same thing as chan_sip and will be disabled by default just like chan_sip. When disabled it does not send the extraneous 180/183 messages. The connected line information simply waits until there is a real reason to send the 180/183 messages. e.g., Such as when the outgoing channel receives a 180/183 message. When the outgoing channel receives a 180/183 message, that message may also contain updated/correct connected line information from the peer. When enabled it sends the connected line update information *immediately* on a 180/183 message regardless of if the far end has received the call yet or the connected line information came from the far end. The core may do some filtering to eliminate an update event if the connected line information does not change. However, when the peer receives a 180/183 message the caller will also get the 180/183 message generated because of the ast_indicate(AST_CONTROL_RINGING/AST_CONTROL_PROGRESS). The key word is immediate. I suppose I should sprinkle immediate a few more times in the option description. I doubt anyone even enables the option in chan_sip because it is undocumented, disabled by default, and will cause this odd behaviour when enabled. These are the reasons why I think the code the option controls should just be removed and the option not even exist. The code doesn't really help and the extraneous 180/183 messages cause problems when enabled. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 13, 2015, 12:13 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 12:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end even gets the call. * Many phones tend to grab the first connected line information and refuse to update the display if it changes. The first information is not likely to be correct if the call goes to an endpoint not under the control of the first Asterisk box. When connected line first went into Asterisk in v1.8, chan_sip received an undocumented option rpid_immediate that defaults to disabled. When enabled, the option
Re: [asterisk-dev] [Code Review] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14690 --- Ship it! Ship It! - gareth On March 13, 2015, 5:13 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 5:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end even gets the call. * Many phones tend to grab the first connected line information and refuse to update the display if it changes. The first information is not likely to be correct if the call goes to an endpoint not under the control of the first Asterisk box. When connected line first went into Asterisk in v1.8, chan_sip received an undocumented option rpid_immediate that defaults to disabled. When enabled, the option immediately passes connected line update information to the caller in 180 Ringing or 183 Progress messages as described above. * Added rpid_immediate option to prevent unnecessary 180 Ringing or 183 Progress messages. The default is no to disable sending the unnecessary messages. Diffs - /branches/13/res/res_pjsip/pjsip_configuration.c 432895 /branches/13/res/res_pjsip.c 432895 /branches/13/include/asterisk/res_pjsip.h 432895 /branches/13/configs/samples/pjsip.conf.sample 432895 /branches/13/channels/chan_pjsip.c 432895 Diff: https://reviewboard.asterisk.org/r/4473/diff/ Testing --- * Ran the tests/channels/pjsip testsuite tests. They still pass. * Made a call chain as follows: 100 - * - * - * - 200. With the patch there are no unnecessary messages. Without the patch there were several 180 Ringing messages sent back to the caller. 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] 4473: chan_pjsip: Add rpid_immediate option to prevent unnecessary 180 Ringing messages.
On March 12, 2015, 8:41 p.m., Matt Jordan wrote: 1. For this to go into Asterisk 13, tests will need to be provided that cover the new parameter. (Really, those tests should be written regardless) 2. The CHANGES file will need to get updated with the new option. rmudgett wrote: Actually I'd prefer that the rpid_immediate option not exist at all and the code it controls to just be removed. Sending connected line updates back to the caller _before_ getting connected doesn't really make sense. This is what the REDIRECTING information is supposed to be doing. gareth wrote: I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered? That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history. If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used? rmudgett wrote: OK, that makes sense but the code that rpid_immediate controls doesn't. As described in the review description, that code immediately sends redundant 180 Ringing or 183 Progress messages at best and at worst lies that the other end is ringing with inaccurate connected line information. Matt Jordan wrote: I'm not sure what this setting is attempting to do then. The description of the new setting makes it sound like it controls sending connected line information prior to Answer, which fits gareth's interpretation. Whether or not it sends multiple 180/183 provisional responses does not seem to be implied in the description of the setting. If setting 'rpid_immediate' to 'yes' still results in extraneous 180/183 provisional responses, then I'm not sure this patch fixes the underlying issue. rmudgett wrote: The option does exactly the same thing as chan_sip and will be disabled by default just like chan_sip. When disabled it does not send the extraneous 180/183 messages. The connected line information simply waits until there is a real reason to send the 180/183 messages. e.g., Such as when the outgoing channel receives a 180/183 message. When the outgoing channel receives a 180/183 message, that message may also contain updated/correct connected line information from the peer. When enabled it sends the connected line update information *immediately* on a 180/183 message regardless of if the far end has received the call yet or the connected line information came from the far end. The core may do some filtering to eliminate an update event if the connected line information does not change. However, when the peer receives a 180/183 message the caller will also get the 180/183 message generated because of the ast_indicate(AST_CONTROL_RINGING/AST_CONTROL_PROGRESS). The key word is immediate. I suppose I should sprinkle immediate a few more times in the option description. I doubt anyone even enables the option in chan_sip because it is undocumented, disabled by default, and will cause this odd behaviour when enabled. These are the reasons why I think the code the option controls should just be removed and the option not even exist. The code doesn't really help and the extraneous 180/183 messages cause problems when enabled. I have rpid_immediate enabled. I know about the option because I searched through chan_sip.c to find out why RPID updates were not working as expected. Until the SIP protocol gains a here is the new RPID info response there is always going to be the problem of piggy-backing that information on a response which already had a meaning. Patch looks fine. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 13, 2015, 5:13 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/ --- (Updated March 13, 2015, 5:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24781 https://issues.asterisk.org/jira/browse/ASTERISK-24781 Repository: Asterisk Description --- Incoming PJSIP call legs that have not been answered yet send unnecessary 180 Ringing or 183 Progress messages every time a connected line update happens. If the outgoing channel is also PJSIP then the incoming channel will always send a 180 Ringing or 183 Progress message when the outgoing channel sends the INVITE. Consequences of these unnecessary messages: * The caller can start hearing ringback before the far end
[asterisk-dev] [Code Review] 4487: AMI PJSIPShowEndpoint closes AMI connection on error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4487/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- AMI PJSIPShowEndpoint closes AMI connection on error This error correction and are very similar to: https://issues.asterisk.org/jira/browse/ASTERISK-24354 Diffs - /trunk/res/res_pjsip/pjsip_configuration.c 432613 Diff: https://reviewboard.asterisk.org/r/4487/diff/ Testing --- Thanks, Dmitriy Serov -- _ -- 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] 4488: Super Awesome Company: Phase 1 - Patch 2 - Outside Connectivity!
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4488/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- Howdy, here is another patch for the Super Awesome Company configuration. We are still in phase 1. The general requirements are posted on the wiki: https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company The specific requirements this patch meets are below: pjsip.conf * SIP ITSP configuration example and have place holders for the required authentication bits. ** Assume that Asterisk does not have a public IP address, and sits behind a NAT with its desk phones. * Have outbound registration to the SIP trunk, and an endpoint that represents the SIP trunk. * Inbound calls received from the SIP trunk should go into their own context. extensions.conf * Match the outbound dial request so that it can only dial US area codes. ** Don't let people dial 900 numbers, international numbers, or any other numbers that could result in a charge * Inbound calls from the SIP trunk should hit a basic Auto Attendant that prompts them for the extension to dial, after greeting them to SAC. * If an inbound call matches a DID that maps to a specific extension/device, dial that extension/device directly. Billing * Make sure CDRs output all calls that are from/to the SIP trunk. These should be logged to a CSV. * For intra-office calls, kill the CDRs. Additional Requirements Noted: * For outbound calls, each SAC employee’s 10-digit DID number is provided as their Caller ID. * 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. * 7, 10 and 10+1 digit dialing for local and long distance calls. * Internal dialing of otherwise inbound features, ** 1100 to reach the main IVR. * The IVR options possible without getting into Phase 2. Diffs - /branches/13/configs/basic-pbx/pjsip.conf 432866 /branches/13/configs/basic-pbx/modules.conf 432866 /branches/13/configs/basic-pbx/logger.conf 432866 /branches/13/configs/basic-pbx/extensions.conf 432866 Diff: https://reviewboard.asterisk.org/r/4488/diff/ Testing --- Setup with a Digium Cloud Services trunk and a few internal phones. Internal to Internal calls. Calls Internal to voicemail and other features. External to internal DID calls. External to internal feature calls. Basically tried to call as many ways as I could through all the various features. Everything seemed to work. Thanks, rnewton -- _ -- 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] 4488: Super Awesome Company: Phase 1 - Patch 2 - Outside Connectivity!
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4488/#review14676 --- /branches/13/configs/basic-pbx/extensions.conf https://reviewboard.asterisk.org/r/4488/#comment25236 Blob, oops. /branches/13/configs/basic-pbx/logger.conf https://reviewboard.asterisk.org/r/4488/#comment25237 This snuck in there during troubleshooting apparently. I should comment that back out. - rnewton On March 13, 2015, 2:32 p.m., rnewton wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4488/ --- (Updated March 13, 2015, 2:32 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Howdy, here is another patch for the Super Awesome Company configuration. We are still in phase 1. The general requirements are posted on the wiki: https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company The specific requirements this patch meets are below: pjsip.conf * SIP ITSP configuration example and have place holders for the required authentication bits. ** Assume that Asterisk does not have a public IP address, and sits behind a NAT with its desk phones. * Have outbound registration to the SIP trunk, and an endpoint that represents the SIP trunk. * Inbound calls received from the SIP trunk should go into their own context. extensions.conf * Match the outbound dial request so that it can only dial US area codes. ** Don't let people dial 900 numbers, international numbers, or any other numbers that could result in a charge * Inbound calls from the SIP trunk should hit a basic Auto Attendant that prompts them for the extension to dial, after greeting them to SAC. * If an inbound call matches a DID that maps to a specific extension/device, dial that extension/device directly. Billing * Make sure CDRs output all calls that are from/to the SIP trunk. These should be logged to a CSV. * For intra-office calls, kill the CDRs. Additional Requirements Noted: * For outbound calls, each SAC employee’s 10-digit DID number is provided as their Caller ID. * 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. * 7, 10 and 10+1 digit dialing for local and long distance calls. * Internal dialing of otherwise inbound features, ** 1100 to reach the main IVR. * The IVR options possible without getting into Phase 2. Diffs - /branches/13/configs/basic-pbx/pjsip.conf 432866 /branches/13/configs/basic-pbx/modules.conf 432866 /branches/13/configs/basic-pbx/logger.conf 432866 /branches/13/configs/basic-pbx/extensions.conf 432866 Diff: https://reviewboard.asterisk.org/r/4488/diff/ Testing --- Setup with a Digium Cloud Services trunk and a few internal phones. Internal to Internal calls. Calls Internal to voicemail and other features. External to internal DID calls. External to internal feature calls. Basically tried to call as many ways as I could through all the various features. Everything seemed to work. Thanks, rnewton -- _ -- 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] 4474: core: Add basic DNS API implementation
On March 11, 2015, 6:05 p.m., rmudgett wrote: /trunk/include/asterisk/dns_recurring.h, line 48 https://reviewboard.asterisk.org/r/4474/diff/1/?file=71969#file71969line48 Since DNS resolution can take time, should the next refresh interval be set in advance of the lowest returned TTL so the refresh happens before a record's expiry? Would it be advisable for the resolver to supply some kind of refresh before TTL expiry? Or add a refresh at xx% of lowest TTL or one second whichever is greater parameter? Maybe a maximum refresh interval time if the TTL is very large. I've updated the documentation to make this clear but I'm hesitant on adding knobs without actually seeing it in use. Primarily because there is no intelligent way to get a good time and no guarantee that it'll actually do anything. If you are using a caching server (which many of us are) it's just going to give you the same result. The unbound resolver itself also acts as a caching resolver so it will just give you the same result back as well. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4474/#review14641 --- On March 11, 2015, 11:42 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4474/ --- (Updated March 11, 2015, 11:42 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24834 https://issues.asterisk.org/jira/browse/ASTERISK-24834 Repository: Asterisk Description --- This change implements the basic API as described on the DNS API wiki page. Minimal changes have been made as required based on real usage and getting a feel for it. As it is the core functionality is present. Resolvers can register, queries can be made (async / sync). As the API was basically copy/pasted from the wiki page there still remain stubs to be filled in for higher level functionality (such as parsing and query sets). Diffs - /trunk/main/dns_query_set.c PRE-CREATION /trunk/main/dns_core.c PRE-CREATION /trunk/include/asterisk/dns_tlsa.h PRE-CREATION /trunk/include/asterisk/dns_srv.h PRE-CREATION /trunk/include/asterisk/dns_resolver.h PRE-CREATION /trunk/include/asterisk/dns_recurring.h PRE-CREATION /trunk/include/asterisk/dns_query_set.h PRE-CREATION /trunk/include/asterisk/dns_naptr.h PRE-CREATION /trunk/include/asterisk/dns_internal.h PRE-CREATION /trunk/include/asterisk/dns_core.h PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4474/diff/ Testing --- Ran DNS unit tests as done by Mark, they are happy. Thanks, Joshua Colp -- _ -- 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] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Justin T. Gibbs wrote: Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c Ed Hynan wrote: OpenBSD and NetBSD. These do not have the user event filter. Ed Hynan wrote: I adapted a test program so see if uncollected EVFILT_TIMER events can make poll() return -- reliably. After testing on FreeBSD 9.0 and OpenBSD 5.5 (leaving NetBSD alone unless something seemed promising), I find that both will fail to return from poll initially; also both can be 'kicked' with a signal or a EVFILT_READ event -- but the result differs on the two systems, and this is undocumented and almost certainly undefined behavior and a side effect (and I assume the EVFILT_USER event was just a similar 'kick'). I haven't studied this timing code, so I shouldn't say much, but *if* res_timing_kqueue.c expects to return a kqueue fd for use with poll, and that poll will wake for EVFILT_TIMER expire events, then that won't work (judging by the test program trying to do just that). The technique in res_timing_pthread.c looks promising: return the read end of a pipe, signal poll by writing a byte, unsignal by consuming the byte (works in the test), and just using EVFILT_TIMER for the ticks. Justin T. Gibbs wrote: I'm not sure what you mean by 'kick'. I believe that EVFILT_TIMER events are reliable. If the application is slow to consume timer events, the kevent will accumulate them and the file descriptor for the kqueue() cause poll to return immediately. The goal of this change is to ensure that continuous mode is cheap and activates the file descriptor immediately. Setting the timer to run as quickly as possible just burns CPU cycles and means continuous mode blocks until the timer goes off at least once. Depending on the timer resolution of the system, this could be a long time. I need to do some more testing with the original driver to quantify this, but I believe this was the cause of my failures on FreeBSD. I coded up a version to use EVFILT_READ on a file descriptor of a closed pipe. This worked on FreeBSD. But since it consumes another fd, I think its still better to use EVFLT_USER if it is available. Yes, EVFILT_TIMER events are reliable. I mean the interaction of kqueue with a timer and poll -- the expectation that poll will return for a kqueue timer expiration. kqueue_timer_fd(), assigned to ast_timing_interface kqueue_timing.timer_fd in svn trunk source, returns the kqueue descriptor (timer-handle) -- this code was not present in 11.* sources, so I haven't had the possible problems, but *if* the descriptor returned from ast_timing_interface kqueue_timing.timer_fd() is used with poll (e.g. main/timing.c), expectations might/will fail to be met. By kick I mean that in tests after poll initially fails to return for kqueue timer expiration, some other events e.g. a signal might be followed by poll suddenly starting to return for kqueue timer expiration -- like something stuck was kicked loose. My guess is that the user event you added was doing just that; but it seems to be unexpected side effect behavior. Regardless of all that, the idea that it's better to use EVFLT_USER if it is available would lead to substantially different code for the kqueue systems. Of course, it's up to asterisk-devs whether that is acceptable (I'm a contributor). - Ed --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 13, 2015, 2:07 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 13, 2015, 2:07 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent().
Re: [asterisk-dev] [Code Review] 4470: Testsuite: Generate UserEvent upon PJsua phones being ready.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4470/ --- (Updated March 13, 2015, 10:43 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Mark Michelson. Changes --- Committed in revision 6523 Repository: testsuite Description --- The test that has been modified on this review would sometimes fail due to the PJsua phone not being ready to receive calls even though it had registered to Asterisk. The changes here resolve the issue by generating a UserEvent on all AMI connections when all PJsua phones are ready. Originating calls to the PJsua phones is now triggered by the 'PJsuaPhonesReady' UserEvent instead of the registration event. Diffs - /asterisk/trunk/tests/rest_api/applications/channel-subscriptions/originate_to_dialplan/non_local_channels/test-config.yaml 6506 /asterisk/trunk/sample-yaml/phones-config.yaml.sample PRE-CREATION /asterisk/trunk/lib/python/asterisk/phones.py 6506 Diff: https://reviewboard.asterisk.org/r/4470/diff/ Testing --- * Executed each test successfully 50+ times on local system and build agent where the test would sometimes fail. 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] 4487: AMI PJSIPShowEndpoint closes AMI connection on error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4487/#review14678 --- Ship it! Ship It! - rmudgett On March 13, 2015, 2:28 a.m., Dmitriy Serov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4487/ --- (Updated March 13, 2015, 2:28 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- AMI PJSIPShowEndpoint closes AMI connection on error This error correction and are very similar to: https://issues.asterisk.org/jira/browse/ASTERISK-24354 Diffs - /trunk/res/res_pjsip/pjsip_configuration.c 432613 Diff: https://reviewboard.asterisk.org/r/4487/diff/ Testing --- Thanks, Dmitriy Serov -- _ -- 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] 4462: DNS core unit tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4462/#review14679 --- Ship it! Ship It! - Joshua Colp On March 6, 2015, 9:02 p.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4462/ --- (Updated March 6, 2015, 9:02 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This provides unit tests for the DNS core as described here: https://wiki.asterisk.org/wiki/display/~jcolp/Asterisk+DNS+API By core this means the bare-bones functionality, such as being able to set and retrieve data on DNS queries. This also includes a mock resolver, whose intention is to ensure that resolver methods are called into when expected. If you have ideas for tests that have not been included here, please mention them in your reviews. Some things that are not covered here: * Recurring asynchronous queries, query sets, NAPTR, SRV, and TLSA are not covered by these tests. These are higher-level APIs on top of the DNS core and will be covered in separate test files. * Nominal asynchronous DNS cancellation is tested here, but off-nominal is not. Off-nominal asynchronous cancellation falls into two basic categories: canceling when there is no query in flight and canceling after a query has completed. You can't test canceling when there is no query in flight because putting the query in flight is what gives you the query object that you would attempt to cancel in the first place. Testing canceling after the query has completed does not test the DNS core as much as it does a specific resolver implementation. Since the resolver implementation is in charge of threading, the core does not try to make any determination of whether it makes sense to be canceling a query or not. Diffs - /trunk/tests/test_dns.c PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4462/diff/ Testing --- All tests pass consistently, and they do not leak memory (as evidenced by MALLOC_DEBUG) Thanks, Mark Michelson -- _ -- 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] 4474: core: Add basic DNS API implementation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4474/ --- (Updated March 13, 2015, 4:04 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24834 https://issues.asterisk.org/jira/browse/ASTERISK-24834 Repository: Asterisk Description --- This change implements the basic API as described on the DNS API wiki page. Minimal changes have been made as required based on real usage and getting a feel for it. As it is the core functionality is present. Resolvers can register, queries can be made (async / sync). As the API was basically copy/pasted from the wiki page there still remain stubs to be filled in for higher level functionality (such as parsing and query sets). Diffs (updated) - /trunk/main/dns_tlsa.c PRE-CREATION /trunk/main/dns_srv.c PRE-CREATION /trunk/main/dns_recurring.c PRE-CREATION /trunk/main/dns_query_set.c PRE-CREATION /trunk/main/dns_naptr.c PRE-CREATION /trunk/main/dns_core.c PRE-CREATION /trunk/include/asterisk/dns_tlsa.h PRE-CREATION /trunk/include/asterisk/dns_srv.h PRE-CREATION /trunk/include/asterisk/dns_resolver.h PRE-CREATION /trunk/include/asterisk/dns_recurring.h PRE-CREATION /trunk/include/asterisk/dns_query_set.h PRE-CREATION /trunk/include/asterisk/dns_naptr.h PRE-CREATION /trunk/include/asterisk/dns_internal.h PRE-CREATION /trunk/include/asterisk/dns_core.h PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4474/diff/ Testing --- Ran DNS unit tests as done by Mark, they are happy. Thanks, Joshua Colp -- _ -- 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] 4471: Recurring asynchronous DNS query unit tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4471/#review14681 --- Ship it! Besides the memory leak I accidentally gave you these look good to go. - Joshua Colp On March 10, 2015, 8:53 p.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4471/ --- (Updated March 10, 2015, 8:53 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- These are more unit tests for DNS, this time for recurring asynchronous queries. There are four tests here that test nominal and off-nominal recurring queries, as well as canceling recurring queries. Like with the core DNS unit tests, I do not have off-nominal cancellation tests because these would test the resolver rather than the DNS core. Diffs - /trunk/tests/test_dns_recurring.c PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4471/diff/ Testing --- These tests pass! Thanks, Mark Michelson -- _ -- 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] 4484: testsuite: Fix HEP tests that fail after fixing pjsip.conf type=global default application.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4484/ --- (Updated March 13, 2015, 10:57 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 6524 Repository: testsuite Description --- Fixing ASTERISK-24807 broke the testsuite HEP tests because a header now appears that didn't before. Diffs - /asterisk/trunk/tests/hep/pjsip/test-config.yaml 6522 /asterisk/trunk/tests/hep/pjsip/configs/ast1/pjsip.conf 6522 /asterisk/trunk/tests/hep/pjsip-ipv6/test-config.yaml 6522 /asterisk/trunk/tests/hep/pjsip-ipv6/configs/ast1/pjsip.conf 6522 /asterisk/trunk/tests/hep/pjsip-auth/test-config.yaml 6522 /asterisk/trunk/tests/hep/pjsip-auth/configs/ast1/pjsip.conf 6522 Diff: https://reviewboard.asterisk.org/r/4484/diff/ Testing --- Tests now pass with the patch. 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] 4471: Recurring asynchronous DNS query unit tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4471/#review14680 --- These tests don't currently release their reference to the recurring query, causing it to get left behind. - Joshua Colp On March 10, 2015, 8:53 p.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4471/ --- (Updated March 10, 2015, 8:53 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- These are more unit tests for DNS, this time for recurring asynchronous queries. There are four tests here that test nominal and off-nominal recurring queries, as well as canceling recurring queries. Like with the core DNS unit tests, I do not have off-nominal cancellation tests because these would test the resolver rather than the DNS core. Diffs - /trunk/tests/test_dns_recurring.c PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4471/diff/ Testing --- These tests pass! Thanks, Mark Michelson -- _ -- 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] 4472: chan_pjsip/res_pjsip_callerid: Make Party ID handling simpler and consistent.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4472/ --- (Updated March 13, 2015, 11:26 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432892 Repository: Asterisk Description --- The res_pjsip modules were manually checking both name and number presentation values when there is a function that determines the combined presentation for a party ID struct. The function takes into account if the name or number components are valid while the manual code rarely checked if the data was even valid. * Made use ast_party_id_presentation() rather than manually checking party ID presentation values. * Ensure that set_id_from_pai() and set_id_from_rpid() will not return presentation values other than what is pulled out of the SIP headers. It is best if the code doesn't assume that AST_PRES_ALLOWED and AST_PRES_USER_NUMBER_UNSCREENED are zero. * Fixed copy paste error in add_privacy_params() dealing with RPID privacy. * Pulled the id-number.valid test from add_privacy_header() and add_privacy_params() up into the parent function add_id_headers() to skip adding PAI/RPID headers earlier. * Made update_connected_line_information() not send out connected line updates if the connected line number is invalid. Lower level code would not add the party ID information and thus the sent message would be unnecessary. * Eliminated RAII_VAR usage in send_direct_media_request(). Diffs - /branches/13/res/res_pjsip_caller_id.c 432722 /branches/13/channels/chan_pjsip.c 432722 Diff: https://reviewboard.asterisk.org/r/4472/diff/ Testing --- Ran the tests/channels/pjsip testsuite tests. They still pass. 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] 4487: AMI PJSIPShowEndpoint closes AMI connection on error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4487/ --- (Updated March 13, 2015, 12:05 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432894 Repository: Asterisk Description --- AMI PJSIPShowEndpoint closes AMI connection on error This error correction and are very similar to: https://issues.asterisk.org/jira/browse/ASTERISK-24354 Diffs - /trunk/res/res_pjsip/pjsip_configuration.c 432613 Diff: https://reviewboard.asterisk.org/r/4487/diff/ Testing --- Thanks, Dmitriy Serov -- _ -- 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
