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/#review15145 --- Ship it! Ship It! - Matt Jordan On April 7, 2015, 3:38 p.m., Y Ateya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/ --- (Updated April 7, 2015, 3:38 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
On April 7, 2015, 10:46 a.m., 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. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/#review15090 --- On April 3, 2015, 2: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, 2: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/#review15090 --- trunk/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4536/#comment25768 I would put a comment in here explaining why the schedule times are chosen as they are. 5/6 feels arbitrary without some explanation. trunk/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4536/#comment25767 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 { ... } - Matt Jordan On April 3, 2015, 2: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, 2: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
On April 7, 2015, 10:46 a.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. Y Ateya wrote: 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? I think your comment that it is somewhat arbitrary, but less than 1 and what we used with registration renewals is both accurate and satisfactory. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4536/#review15090 --- On April 3, 2015, 2: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, 2: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
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] 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
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. Just going to chime in with my agreement here. I'm not sure how max_retries fits into this. - Mark --- 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
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/#review15007 --- trunk/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4536/#comment25667 Restore this line please. trunk/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4536/#comment25671 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. - Matt Jordan On March 26, 2015, 6: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, 6: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