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
