Re: [asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 9, 2015, 2:21 p.m.) Review request for Asterisk Developers. Changes --- - Removed useless log message - Used sscanf instead of atoi Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Changes: - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const. - Add general configuration variable `calltokenexpiration` Diffs (updated) - trunk/configs/samples/iax.conf.sample 432806 trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- 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] 4483: Separate QoS settings for trunk packets
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4483/ --- (Updated April 9, 2015, 7:27 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers and mattjordan. Bugs: ASTERISK-24866 https://issues.asterisk.org/jira/browse/ASTERISK-24866 Repository: Asterisk Description --- Currently the packet priority is assigned to all packets sent from IAX socket; Which minimizes the benefit of having packet priority. This patch adds separate qos settings for IAX trunk packets. Summary: - Added trunk_cos and trunk_tos configuraiton variables (similar to cos and tos) to set qos values for trunk packets. - Added dynamic_qos configuration variables (true/false) to enable/disable changing qos dynamically. (default disabled). - Before calling ast_send (and if dynamic_qos is enabled) update qos settings of the socket (if it needs change only). - Lock a mutex before setting qos and unlock it after ast_send. To prevent thread1 setting qos, thread two setting qos and send, then thread1 uses thread2 qos. Notes: - Protecting `qos_dynamic_enabled` in reload configurations complicated locking mechanism a little bit. - More qos classes can be added to different packet types later. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4483/diff/ Testing --- Run 100 calls (over IAX trunk) between client and server. Checked that: - If cos, tos are set but dynamic_qos is disabled, tos and cos values are used. - If tos, cos, dynamic_qos, trunk_cos and trunk_tos are set; trunk packets uses trunk_cos/trunk_tos while all other packets use cos/tos. Thanks, Y Ateya -- _ -- 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] 4536: iax2_poke_noanswer expiration timer too short
On April 7, 2015, 3:46 p.m., Matt Jordan wrote:
Matt Jordan wrote:
Also, please remember to close findings that you feel have been resolved.
Otherwise, it is difficult to know what all has been addressed between
reviews.
ok.
On April 7, 2015, 3:46 p.m., Matt Jordan wrote:
trunk/channels/chan_iax2.c, lines 12365-12375
https://reviewboard.asterisk.org/r/4536/diff/2/?file=73573#file73573line12365
I would put a comment in here explaining why the schedule times are
chosen as they are. 5/6 feels arbitrary without some explanation.
Using *5/6 is **really** arbitrary; I just want number less than maximum value.
This is used because POKE next cycle will be after {{pokefreqnotok}}, so I want
current poke to expire before the next new poke is sent.
So which multiplier to use, 5/6, 9/10, 99/100, /1, ... etc. It needs to
be something less than 1.
I chose 5/6 because it is used in registration renewals {{(5 * reg-refresh /
6)}}
What do you recommend to document this multiplier?
On April 7, 2015, 3:46 p.m., Matt Jordan wrote:
trunk/channels/chan_iax2.c, lines 12367-12372
https://reviewboard.asterisk.org/r/4536/diff/2/?file=73573#file73573line12367
While I know you didn't introduce this, the coding guidelines stipulate
that you should use braces even over single line statements. Macros have
caused us extreme pain in the past.
if (peer-lastms 0) {
...
} else {
...
}
Sure.
- Y
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4536/#review15090
---
On April 3, 2015, 7:58 p.m., Y Ateya wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4536/
---
(Updated April 3, 2015, 7:58 p.m.)
Review request for Asterisk Developers and rnewton.
Bugs: ASTERISK-24894
https://issues.asterisk.org/jira/browse/ASTERISK-24894
Repository: Asterisk
Description
---
Increase POKE retry window from DEFAULT_MAXMS * 2 (4 seconds) to bigger
number (derived from qualify time; which control POKE retry time).
Diffs
-
trunk/channels/chan_iax2.c 432806
Diff: https://reviewboard.asterisk.org/r/4536/diff/
Testing
---
- Tried test with multiple qualify values (2 and 10 seconds).
- Tried test with 100% packets loss to ensure that when a POKE packet is
dropped it will be retried couple of time before declaring client
disconnected.
Thanks,
Y Ateya
--
_
-- 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] 4536: iax2_poke_noanswer expiration timer too short
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/ --- (Updated April 7, 2015, 8:38 p.m.) Review request for Asterisk Developers and rnewton. Changes --- - Comments updated. - Braces used for if-else. Bugs: ASTERISK-24894 https://issues.asterisk.org/jira/browse/ASTERISK-24894 Repository: Asterisk Description --- Increase POKE retry window from DEFAULT_MAXMS * 2 (4 seconds) to bigger number (derived from qualify time; which control POKE retry time). Diffs (updated) - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4536/diff/ Testing --- - Tried test with multiple qualify values (2 and 10 seconds). - Tried test with 100% packets loss to ensure that when a POKE packet is dropped it will be retried couple of time before declaring client disconnected. Thanks, Y Ateya -- _ -- 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] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 10:06 p.m.) Review request for Asterisk Developers. Changes --- Added configuration variable to iax.sample.conf Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Changes: - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const. - Add general configuration variable `calltokenexpiration` Diffs (updated) - trunk/configs/samples/iax.conf.sample 432806 trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- 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] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 9:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- 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] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- 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] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 9:49 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description (updated) --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Changes: - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const. - Add general configuration variable `calltokenexpiration` Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- 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] 4536: iax2_poke_noanswer expiration timer too short
On April 1, 2015, 1:07 a.m., Matt Jordan wrote: trunk/channels/chan_iax2.c, line 12370 https://reviewboard.asterisk.org/r/4536/diff/1/?file=72980#file72980line12370 The usage of max_retries here feels arbitrary. I'm not against this being controlled more dynamically based on the last known qualify time, but I'd rather just see that be 4 or 8 here, as appropriate. Mark Michelson wrote: Just going to chime in with my agreement here. I'm not sure how max_retries fits into this. The purpose of dependence on max_retries (and qualify) was to allow for POKE to be retried max_retries time (based on qualify value); Using a magic number (ex. multiply by 8) don't give any insight about why did you do this. NOTE: As retry time changes after consecutive retries it will do less than max_retries retries before iax2_poke_noanswer expiration. For example: If retry time is 500ms, first retry will be after 500ms, second retry will be 5 seconds (500ms * 10), third retry after 10seconds (5 seconds * 10, cieled to MAX_RETRY_TIME -10s-), fourth retry after 10s. - Y --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/#review15007 --- On March 26, 2015, 11:42 p.m., Y Ateya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/ --- (Updated March 26, 2015, 11:42 p.m.) Review request for Asterisk Developers and rnewton. Bugs: ASTERISK-24894 https://issues.asterisk.org/jira/browse/ASTERISK-24894 Repository: Asterisk Description --- Increase POKE retry window from DEFAULT_MAXMS * 2 (4 seconds) to bigger number (derived from qualify time; which control POKE retry time). Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4536/diff/ Testing --- - Tried test with multiple qualify values (2 and 10 seconds). - Tried test with 100% packets loss to ensure that when a POKE packet is dropped it will be retried couple of time before declaring client disconnected. Thanks, Y Ateya -- _ -- 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] 4536: iax2_poke_noanswer expiration timer too short
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/ --- Review request for Asterisk Developers and rnewton. Bugs: ASTERISK-24894 https://issues.asterisk.org/jira/browse/ASTERISK-24894 Repository: Asterisk Description --- Increase POKE retry window from DEFAULT_MAXMS * 2 (4 seconds) to bigger number (derived from qualify time; which control POKE retry time). Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4536/diff/ Testing --- - Tried test with multiple qualify values (2 and 10 seconds). - Tried test with 100% packets loss to ensure that when a POKE packet is dropped it will be retried couple of time before declaring client disconnected. Thanks, Y Ateya -- _ -- 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] 4483: Separate QoS settings for trunk packets
On March 23, 2015, 7:52 p.m., Matt Jordan wrote: trunk/channels/chan_iax2.c, lines 345-347 https://reviewboard.asterisk.org/r/4483/diff/2/?file=72090#file72090line345 This lock concerns me a lot. Because you are looking to toggle the QOS setting on the socket, you have to protect access to the socket across all threads. That substantially increases the locking on the socket in chan_iax2. At a minimum, this is called from iax2_write, which is called by the PBX threads servicing the channels. As a result, this would globally lock all IAX2 channels on every frame written to any IAX2 channel. While this is an interesting idea, the impact of the implementation here feels rather dramatic. Joshua Colp wrote: +1 to this. Have you run many simultaneous channels with it in trunking and non-trunking mode? @Matt: Yes, the lock is dramatic. I can remove the locks, but some packets will be sent with the wrong qos settings. @Joshua: I tried only trunked calls. - Y --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4483/#review14772 --- On March 12, 2015, 8:11 p.m., Y Ateya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4483/ --- (Updated March 12, 2015, 8:11 p.m.) Review request for Asterisk Developers and mattjordan. Bugs: ASTERISK-24866 https://issues.asterisk.org/jira/browse/ASTERISK-24866 Repository: Asterisk Description --- Currently the packet priority is assigned to all packets sent from IAX socket; Which minimizes the benefit of having packet priority. This patch adds separate qos settings for IAX trunk packets. Summary: - Added trunk_cos and trunk_tos configuraiton variables (similar to cos and tos) to set qos values for trunk packets. - Added dynamic_qos configuration variables (true/false) to enable/disable changing qos dynamically. (default disabled). - Before calling ast_send (and if dynamic_qos is enabled) update qos settings of the socket (if it needs change only). - Lock a mutex before setting qos and unlock it after ast_send. To prevent thread1 setting qos, thread two setting qos and send, then thread1 uses thread2 qos. Notes: - Protecting `qos_dynamic_enabled` in reload configurations complicated locking mechanism a little bit. - More qos classes can be added to different packet types later. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4483/diff/ Testing --- Run 100 calls (over IAX trunk) between client and server. Checked that: - If cos, tos are set but dynamic_qos is disabled, tos and cos values are used. - If tos, cos, dynamic_qos, trunk_cos and trunk_tos are set; trunk packets uses trunk_cos/trunk_tos while all other packets use cos/tos. Thanks, Y Ateya -- _ -- 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] 4483: Separate QoS settings for trunk packets
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4483/ --- Review request for Asterisk Developers and mattjordan. Bugs: ASTERISK-24866 https://issues.asterisk.org/jira/browse/ASTERISK-24866 Repository: Asterisk Description --- Currently the packet priority is assigned to all packets sent from IAX socket; Which minimizes the benefit of having packet priority. This patch adds separate qos settings for IAX trunk packets. Summary: - Added trunk_cos and trunk_tos configuraiton variables (similar to cos and tos) to set qos values for trunk packets. - Added dynamic_qos configuration variables (true/false) to enable/disable changing qos dynamically. (default disabled). - Before calling ast_send (and if dynamic_qos is enabled) update qos settings of the socket (if it needs change only). - Lock a mutex before setting qos and unlock it after ast_send. To prevent thread1 setting qos, thread two setting qos and send, then thread1 uses thread2 qos. Notes: - Protecting `qos_dynamic_enabled` in reload configurations complicated locking mechanism a little bit. - More qos classes can be added to different packet types later. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4483/diff/ Testing --- Run 100 calls (over IAX trunk) between client and server. Checked that: - If cos, tos are set but dynamic_qos is disabled, tos and cos values are used. - If tos, cos, dynamic_qos, trunk_cos and trunk_tos are set; trunk packets uses trunk_cos/trunk_tos while all other packets use cos/tos. Thanks, Y Ateya -- _ -- 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
