Re: [asterisk-dev] [Code Review] 4441: Enable TLS Dual-Certificates (ECC+RSA)
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4441/#review14966 --- Thank you for working on the TLS code, we surely need more attention to that. I am not sure about adding DSA, but adding ECC is a good thing. I would suggest going for more config parameters instead of guessing file names. We are not doing that anywhere else (that I know of) and I don't think it's a good thing. - Olle E Johansson On March 30, 2015, 10:34 a.m., Alexander Traud wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4441/ --- (Updated March 30, 2015, 10:34 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24815 https://issues.asterisk.org/jira/browse/ASTERISK-24815 Repository: Asterisk Description --- Already works for Asterisk as the client. Enables dual- (or triple-) certificates for Asterisk as the TLS server. When a client connects via SSL/TLS, the server uses a RSA key-pair usually. However, more such algorithms exist like DSA and ECDSA. If you go for one of those, you would loose compatibility to RSA-only clients. This patch allows you to provide up-to one RSA, ECDSA and DSA key each (= one key or two keys or three keys). Copied over from the Apache HTTP server project, added in version 2.4.8. Usage: tlscertfile=/etc/asterisk/example_rsa.pem Then, the code of this patch picks that path, filename, and searches for files called example_ecc.pem and example_dsa.pem automatically. Diffs - trunk/main/tcptls.c 431938 trunk/configs/samples/sip.conf.sample 428526 Diff: https://reviewboard.asterisk.org/r/4441/diff/ Testing --- by developer, manually This patch was tested in Ubuntu 14.04 LTS with a certificate from Comodo (ECC; chains-up to AddTrust and UTN) and RapidSSL (RSA; chains-up to GeoTrust and Equifax). TLS clients were CounterPath Bria (BlackBerry) and CSipSimple (Android). The test was done with OpenSSL 1.0.1 and OpenSSL 1.0.2. Both versions work as expected. However, if you use well-known (commercial) certificates, you might use different certificate chains. For this, you need at least OpenSSL 1.0.2. If you use your own certificate authority without a certificate chain, OpenSSL 1.0.1 is sufficient. Because no new symbol of OpenSSL was used, I do not see a reason why this patch should not be compatible with older OpenSSL releases. Therefore, no if/def/version is introduced in this patch. Thanks, Alexander Traud -- _ -- 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] RFC: Refactor qualify and res_pjsip/endpt_send_request
On 30 Mar 2015, at 16:54, Mark Michelson mmichel...@digium.com wrote: On 03/28/2015 08:06 PM, Joshua Colp wrote: George Joseph wrote: The fact that it goes to unavailable would be a bug. Why does it do so? Mark should probably chime in here but I think it's because the earliest you could get a response from pjsip when a contact isn't reachable is the unconfigurable 32 seconds. As I said that's a long time to leave a contact available when it really isn't. Without implementing our own timer setting the contact to unavailable was probably the lesser of 2 evils. No matter what there's going to be a period where you are potentially wrong. I don't think making it unavailable was done on purpose. Actually, I believe the timer may be configurable. In the type=system settings, there are timer_t1 and timer_b settings. timer_t1 is the base used for determining the retransmission interval, and timer_b is the maximum time we will wait before giving up sending the request. The defaults for these values are 500 ms and 32000 ms respectively. If you were to change timer_b to be a smaller value, then presumably you would have a shorter time before the transaction times out. A couple of caveats about these settings 1) Since they're in the type=system settings, any change you make requires an Asterisk restart in order to take effect. Are you serious? That's a very strange design. Without knowing anything about PJSIP, I think that is something that needs to be fixed. There are several SIP phones based on PJSIP where I can set timers without restarting. Wonder how they did it. 2) PJSIP applies these timers globally. They will affect ALL SIP transactions, not just the OPTIONS transactions from the qualify checks. That seems very strange, but I have to trust you here. It makes the channel driver rather unusable in gateway situations where it's a requirement that I have one set of timers for my internal systems and one for external clients. /O -- _ -- 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] 4558: Re-add _ast_mem_backtrace_buffer variable for ABI compatibility
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4558/ --- (Updated March 31, 2015, 6:47 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and George Joseph. Changes --- Committed in revision 433795 Repository: Asterisk Description --- This patch re-adds the _ast_mem_backtrace_buffer variable to ensure ABI compatibility with modules built prior to commit of r4502. This applies to 13 only since the variable didn't exist in 11. Diffs - /branches/13/main/utils.c 433716 Diff: https://reviewboard.asterisk.org/r/4558/diff/ Testing --- Build only, George if you can please verify this resolves your issue loading your DPMA module. 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] running pjsip testsuite
On Tue, Mar 31, 2015 at 8:04 AM, Yaron Nachum nachum.ya...@gmail.com wrote: Hi everyone, I am trying to add some tests for the PJSIP auto-dtmf support . Before I start I just wanted to run some of the existing tests in order to understand the process. Whenever I try to run a test from the pjsip tests I get - --- -- Dependency: res_pjsip - False. The following output was generated when I tried to run the rpid_immediate test: [root@stnrd5652 testsuite]# ./runtests.py -t tests/channels/pjsip/rpid_immediate/ Running tests for Asterisk SVN-trunk-r431522M ... Tests to run: 0, Maximum test inactivity time: -1 sec. -- Cannot run test 'tests/channels/pjsip/rpid_immediate' --- -- Minimum Version: 13.4.0 (True) --- -- Maximum Version: (True) --- -- Tags: ['pjsip'] --- -- Dependency: twisted - True --- -- Dependency: starpy - True --- -- Dependency: app_dial - True --- -- Dependency: app_echo - True --- -- Dependency: func_callerid - True --- -- Dependency: chan_pjsip - False --- -- Dependency: res_pjsip - False --- -- Dependency: res_pjsip_caller_id - False --- -- Dependency: res_pjsip_endpoint_identifier_user - False --- -- Dependency: res_pjsip_sdp_rtp - False --- -- Dependency: res_pjsip_session - False ?xml version=1.0 encoding=utf-8? testsuite errors=0 failures=0 name=AsteriskTestSuite tests=0 time=0.00/ Am I doing anything stupid? Congratulations on getting this far! Looks like you've got most of the dependencies worked out in the testsuite. The dependency checking for 'res_pjsip' is looking at what modules you have installed on the system. Double check that your Asterisk installation did detect pjproject, and that it built and installed the res_pjsip* modules. You can check this in menuselect. Matt -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15013
---
/branches/13/channels/chan_skinny.c
https://reviewboard.asterisk.org/r/4533/#comment25681
Red blob
/branches/13/channels/chan_skinny.c
https://reviewboard.asterisk.org/r/4533/#comment25682
Guidelines: No space with parens
if (a) {
}
/branches/13/include/asterisk/cel.h
https://reviewboard.asterisk.org/r/4533/#comment25683
Remove
/branches/13/include/asterisk/utils.h
https://reviewboard.asterisk.org/r/4533/#comment25684
(int) (v)
What if v were an expression and not a variable? In that case you would
cast the wrong thing. That is why with macros you put parens around parameter
references and the overall macro expression.
#define MACRO(a,b) ((a) + (b))
- rmudgett
On March 31, 2015, 8:59 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated March 31, 2015, 8:59 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not make
sence. Not 100% if encode_length has side effects, so left the actual call
the this function in place. (Needs to be rechecked by code-owner)
/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from
ast_hint_presence_state.. Not very nice, but did not want to change the
signature of this function.
/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be 0
/branches/13/res/res_stasis_recording.c:598/604
recording-state / operation is enum and cannot be 0
/branches/13/res/res_security_log.c:992
enum event_type cannot be 0
use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the
lower limit which is always true.
Diffs
-
/branches/13/res/res_stasis_recording.c 433444
/branches/13/res/res_stasis_playback.c 433444
/branches/13/res/res_pjsip_exten_state.c 433444
/branches/13/res/ari/resource_channels.c 433444
/branches/13/res/ari/resource_bridges.c 433444
/branches/13/main/udptl.c 433444
/branches/13/main/security_events.c 433444
/branches/13/main/presencestate.c 433444
/branches/13/main/indications.c 433444
/branches/13/main/event.c 433444
/branches/13/main/enum.c 433444
/branches/13/main/cel.c 433444
/branches/13/main/app.c 433444
/branches/13/include/asterisk/utils.h 433444
/branches/13/include/asterisk/logger.h 433444
/branches/13/include/asterisk/cel.h 433444
/branches/13/include/asterisk/app.h 433444
/branches/13/funcs/func_curl.c 433444
/branches/13/channels/pjsip/dialplan_functions.c 433444
/branches/13/channels/chan_skinny.c 433444
Diff: https://reviewboard.asterisk.org/r/4533/diff/
Testing
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 4:52 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs clang compiler warning:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 4:54 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs clang compiler warning:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 4:30 a.m., rmudgett wrote:
/branches/13/channels/chan_skinny.c, line 2387
https://reviewboard.asterisk.org/r/4533/diff/8/?file=73424#file73424line2387
Guidelines: No space with parens
if (a) {
}
Getting late...
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15013
---
On April 1, 2015, 4:52 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated April 1, 2015, 4:52 a.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not make
sence. Not 100% if encode_length has side effects, so left the actual call
the this function in place. (Needs to be rechecked by code-owner)
/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from
ast_hint_presence_state.. Not very nice, but did not want to change the
signature of this function.
/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be 0
/branches/13/res/res_stasis_recording.c:598/604
recording-state / operation is enum and cannot be 0
/branches/13/res/res_security_log.c:992
enum event_type cannot be 0
use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the
lower limit which is always true.
Diffs
-
/branches/13/res/res_stasis_recording.c 433444
/branches/13/res/res_stasis_playback.c 433444
/branches/13/res/res_pjsip_exten_state.c 433444
/branches/13/res/ari/resource_channels.c 433444
/branches/13/res/ari/resource_bridges.c 433444
/branches/13/main/udptl.c 433444
/branches/13/main/security_events.c 433444
/branches/13/main/presencestate.c 433444
/branches/13/main/indications.c 433444
/branches/13/main/event.c 433444
/branches/13/main/enum.c 433444
/branches/13/main/cel.c 433444
/branches/13/main/app.c 433444
/branches/13/include/asterisk/utils.h 433444
/branches/13/include/asterisk/logger.h 433444
/branches/13/include/asterisk/cel.h 433444
/branches/13/include/asterisk/app.h 433444
/branches/13/funcs/func_curl.c 433444
/branches/13/channels/pjsip/dialplan_functions.c 433444
/branches/13/channels/chan_skinny.c 433444
Diff: https://reviewboard.asterisk.org/r/4533/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] 4533: clang compiler warning: -Wtautological-compare
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15003
---
/branches/13/channels/chan_skinny.c
https://reviewboard.asterisk.org/r/4533/#comment25666
Since one of the definitions of letohl() defined above could return a
signed value, it is best to assign the returned value to an unsigned variable
and then compare.
unsigned long len;
len = letohl(req-len)
if (SKINNY_MAX_PACKET len) {
}
/branches/13/funcs/func_curl.c
https://reviewboard.asterisk.org/r/4533/#comment25672
Try defining this as:
#define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) -500)
This should shut-up the compiler without changing the binary value.
/branches/13/include/asterisk/app.h
https://reviewboard.asterisk.org/r/4533/#comment25650
This changes ABI by changing the other enum values.
Change to:
AST_RECORD_IF_EXISTS_ERROR = -1,
This takes the place of plain -1 when it is used in conjunction with this
enum.
/branches/13/include/asterisk/cel.h
https://reviewboard.asterisk.org/r/4533/#comment25652
You are missing two values:
AST_CEL_INVALID_VALUE = -1,
AST_CEL_ALL = 0,
Use AST_CEL_INVALID_VALUE in place of AST_CEL_SENTINEL.
/branches/13/include/asterisk/utils.h
https://reviewboard.asterisk.org/r/4533/#comment25654
You should be able to do this cast change unconditionally.
/branches/13/include/asterisk/utils.h
https://reviewboard.asterisk.org/r/4533/#comment25653
Put parens around the use of v to cast what you think you are casting.
/branches/13/main/app.c
https://reviewboard.asterisk.org/r/4533/#comment25660
This should never happen so replace with an
ast_assert(0);
/branches/13/main/cel.c
https://reviewboard.asterisk.org/r/4533/#comment25656
change 0 to AST_CEL_ALL
/branches/13/main/cel.c
https://reviewboard.asterisk.org/r/4533/#comment25655
You have now duplicated this error message in ast_cel_str_to_event_type().
/branches/13/res/res_pjsip_exten_state.c
https://reviewboard.asterisk.org/r/4533/#comment25658
Guidelines: No declarations within code area. Only at beginning of a
statement block.
/branches/13/res/res_pjsip_exten_state.c
https://reviewboard.asterisk.org/r/4533/#comment25659
Pull the assignment within the test out.
presence_state = ast_hint_presence_state();
if (presence_state == -1) {
}
I have a feeling that presence_state should be checked for
AST_PRESENCE_INVALID as an error in addition to -1. However, I'm not sure.
- rmudgett
On March 30, 2015, 6:08 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated March 30, 2015, 6:08 a.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15012 --- /branches/13/tests/test_strings.c https://reviewboard.asterisk.org/r/4555/#comment25680 De red blobs. - rmudgett On March 31, 2015, 8:30 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 31, 2015, 8:30 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:47 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
On April 1, 2015, 1:21 a.m., rmudgett wrote:
/branches/13/main/utils.c, lines 492-493
https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492
Revert this. This change could affect the callers of the function
since you are changing the API. However, no callers currently care about
the return value.
You changed this because clang had a problem in test_semi1() in
tests/test_strings.c. There is a better way.
Diederik de Groot wrote:
Actually as i tried to mention in the description i changed this
according to the tests being run. I was not sure which one is supposed to be
leading, the test or the source.
The test says it expects:
test_semi(;, , 0)
To be legal. FOr it to be legal ast_alloca(0) must be allowed.
Am i really changing the API here ?
rmudgett wrote:
You are changing what is returned if buflen is zero. Before it would
return outbuf while the change makes it return NULL. It is just fortunate
that nothing cares about the return value.
The test code is in error here because for a zero length it writes beyond
the buffer boundary.
Ok Agreed.
On April 1, 2015, 1:21 a.m., rmudgett wrote:
/branches/13/tests/test_strings.c, lines 395-396
https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395
Is clang returning a NULL pointer when passed a zero length? If so
this could be a problem througout the codebase since the code assumes that
the function can never fail.
Diederik de Groot wrote:
I am not sure what happens, the compiler actually segfaults when it
encountered this one. I was a little shocked about it as well. I guess the
__builtins are a little more scary. If you wanna find out, give it a try. I
think this should at least be reported to the llvm/clang team.
I am not sure how to interpret alloca(0) either... What is the developer
trying to achieve here. And what should it return. It can't allocate space of
0 length and return a pointer to it.
A simple but ugly temp-fix would be to change
#define ast_alloca(size) __builtin_alloca(size)
to:
#if defined(__clang)
#define ast_alloca(size) __builtin_alloca(size ? size : 1);
#else
#define ast_alloca(size) __builtin_alloca(size)
#endif
rmudgett wrote:
That could be an undefined area. What is the return value of malloc(0)?
Some systems return NULL while other systems return a pointer that you cannot
dereference but can later pass to free().
The problem with the original test_semi() code is that with a zero length
we dereference any returned pointer to put a '\0' in it. For a zero length
buffer that is beyond the bounds of the buffer and will corrupt the stack.
The code also reads beyond the boundary in strcmp().
Wrote a little tester.c
#include malloc.h
#include stdio.h
#include strings.h
int main() {
int *test1 = malloc(0);
int *test2 = __builtin_alloca(0);
printf(%p / %p\n, test1, test2);
free(test1);
}
And without any compiler flags, clang and gcc seem to be happy and running this
without a problem. Will have to figure out which compiler flags the Makefile
compounds to see if that makes any difference.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/#review15000
---
On April 1, 2015, 4:47 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/
---
(Updated April 1, 2015, 4:47 a.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
fixes for tests to be compiled using clang
Diffs
-
/branches/13/tests/test_strings.c 433444
/branches/13/tests/test_stringfields.c 433444
/branches/13/tests/test_sched.c 433444
/branches/13/tests/test_acl.c 433444
Diff: https://reviewboard.asterisk.org/r/4555/diff/
Testing
---
executing the tests one-by-one works fine (completes to end) (skipping
/main/stdtime) -
test show results failed:
=== /main/message/ ==
FAIL test_message_queue_handler_nom /main/message/ 31036ms
[test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test
timed out while waiting for handler to get message
Not sure if this is actually a fail or just a
Re: [asterisk-dev] [Code Review] 4551: clang compiler warning: -Wnon-literal-null-conversion
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4551/#review15006 --- Ship it! Ship It! - Matt Jordan On March 30, 2015, 6:16 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4551/ --- (Updated March 30, 2015, 6:16 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wnon-literal-null-conversion Diffs - /branches/13/res/stasis_recording/stored.c 433444 /branches/13/channels/chan_skinny.c 433444 /branches/13/channels/chan_sip.c 433444 Diff: https://reviewboard.asterisk.org/r/4551/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4571: Add usegmtime option to cel_pgsql
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/#review15004
---
/trunk/CHANGES
https://reviewboard.asterisk.org/r/4571/#comment25661
Two findings here:
1) The review shows a red blob. Extraneous white space should be deleted.
2) The formatting and wording here doesn't match other sections. I would
use:
cel_pgsql
--
* Added a new option, 'usegmtime', which causes timestamps in CEL events
to be logged in GMT.
/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25662
There's no reason to initialize this to 0. static variables are allocated
in the global data segment, and are always initialized to 0.
Technically, this applies to the 'connected' variable as well, but that's
outside the scope of this review :-)
/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25663
You actually don't need the variable. See comment below.
/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25664
You don't need the newusegmtime variable here. Instead, this should simply
be written as:
tmp = ast_variable_retrieve(cfg, global, usegmtime));
if (tmp) {
usegmtime = ast_true(tmp);
} else {
usegmtime = 0;
}
Note that this assumes that not setting the 'usegmtime' variable should
also clear whatever the previous value to its default of False - which should
probably be the case, since that is the default value.
/trunk/configs/cel_pgsql.conf.sample
https://reviewboard.asterisk.org/r/4571/#comment25665
Extraneous white space.
- Matt Jordan
On March 30, 2015, 8:19 p.m., Rodrigo Ramirez Norambuena wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/
---
(Updated March 30, 2015, 8:19 p.m.)
Review request for Asterisk Developers.
Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186
https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186
Repository: Asterisk
Description
---
Feature for added an option that lets you specify that the timestamps going
into PostgreSQL from CEL log should be in GMT instead of local time.
Diffs
-
/trunk/configs/cel_pgsql.conf.sample 406488
/trunk/cel/cel_pgsql.c 406488
/trunk/CHANGES 406488
Diff: https://reviewboard.asterisk.org/r/4571/diff/
Testing
---
Thanks,
Rodrigo Ramirez Norambuena
--
_
-- 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] 4554: clang compiler warning: -Wunused-value
On April 1, 2015, 12:30 a.m., rmudgett wrote: I dislike this warning. It is more nuisance than helpful. I once worked with compiler that had a similar warning. The lengths you sometimes have to go to eradicate the the warning can be extream. There are several places in this patch that change code behavior and introduce bugs. Hi rmudgett, I agree this would be the biggest amount of work of the whole 'clang' project. It does not really matter from a performance standpoint i think. It can be helpfull to find refcount issues where a previously used refcounted pointer was unreffed and might vanish at any moment. If we where to always require that you have to use the return value from any function including XXX_unref, then we would be setting that that pointer to NULL and therefor the static compiler would be able to detect the null pointer dereference for any use after this location. There are other ways to deal with this, for example adding __attribute(context)__ and using sparse to find these (like the linux-kernel does) instead of depending on this little warning. Is it worth the work ? I don't know. I leave that decision up to you guys. You might want to have a look at: http://asterisk.chan-sccp.net:443/scanbuild-output/2015-03-29-155615-16015-1/ To see how usefull this static compiler can really be (Don't mind the Cast from non-struct type to struct type for now). FYI : I know my patch/fix was certainly not complete nor quite sure not bug free, it was more a sample case of how this could be handled, sorry that that was not clear from the patch description. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14999 --- On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, line 399 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line399 Did you mean ast_alloca() or ast_strdupa()? Ooh sorry i meant ast_alloca... Typo. On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/main/utils.c, lines 492-493 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492 Revert this. This change could affect the callers of the function since you are changing the API. However, no callers currently care about the return value. You changed this because clang had a problem in test_semi1() in tests/test_strings.c. There is a better way. Actually as i tried to mention in the description i changed this according to the tests being run. I was not sure which one is supposed to be leading, the test or the source. The test says it expects: test_semi(;, , 0) To be legal. FOr it to be legal ast_alloca(0) must be allowed. Am i really changing the API here ? On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, lines 395-396 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395 Is clang returning a NULL pointer when passed a zero length? If so this could be a problem througout the codebase since the code assumes that the function can never fail. I am not sure what happens, the compiler actually segfaults when it encountered this one. I was a little shocked about it as well. I guess the __builtins are a little more scary. If you wanna find out, give it a try. I think this should at least be reported to the llvm/clang team. I am not sure how to interpret alloca(0) either... What is the developer trying to achieve here. And what should it return. It can't allocate space of 0 length and return a pointer to it. A simple but ugly temp-fix would be to change #define ast_alloca(size) __builtin_alloca(size) to: #if defined(__clang) #define ast_alloca(size) __builtin_alloca(size ? size : 1); #else #define ast_alloca(size) __builtin_alloca(size) #endif On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, line 393 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line393 Revert this. On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, lines 407-409 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line407 Revert now that the clang ast_alloca() problem is worked around. ast_alloca problem remain, but is not specifically related to this issue. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15000 --- On April 1, 2015, 3:30 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 3:30 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime ==
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 3:30 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/funcs/func_curl.c, lines 174-175
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73369#file73369line174
Try defining this as:
#define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) -500)
This should shut-up the compiler without changing the binary value.
CurlOption is a positive enum though. So the warning about it not being allowed
to be negative remains, won't it ?
BTW: I made a typo while changing this one
I was meaning to set it to:
#define CURLOPT_SPECIAL_HASHCOMPAT CURLOPT_LASTENTRY
Which is a legal value. Not really happy with this though.
Setting it to
#define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) 500)
Might be an option, not sure if this is checked though.
This would require a little further source investigation.
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/include/asterisk/utils.h, lines 833-837
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73373#file73373line833
You should be able to do this cast change unconditionally.
Just wanted to denote that it was clang specific.
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/res/res_pjsip_exten_state.c, lines 412-413
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73384#file73384line412
Pull the assignment within the test out.
presence_state = ast_hint_presence_state();
if (presence_state == -1) {
}
I have a feeling that presence_state should be checked for
AST_PRESENCE_INVALID as an error in addition to -1. However, I'm not sure.
Added a check for both, seems right. Would have to be checked though, by
someone who knows !
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15003
---
On March 30, 2015, 1:08 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated March 30, 2015, 1:08 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not make
sence. Not 100% if encode_length has side effects, so left the actual call
the this function in place. (Needs to be rechecked by code-owner)
/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from
ast_hint_presence_state.. Not very nice, but did not want to change the
signature of this function.
/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be 0
/branches/13/res/res_stasis_recording.c:598/604
recording-state / operation is enum and cannot be 0
/branches/13/res/res_security_log.c:992
enum event_type cannot be 0
use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the
lower limit which is always true.
Diffs
-
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:46 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15016 --- Ship it! Ship It! - rmudgett On March 31, 2015, 9:42 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 31, 2015, 9:42 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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] [IAX] ping time calculations
On Tue, Mar 24, 2015 at 4:04 PM, Yousf Ateya y.at...@starkbits.com wrote: One of the ways in which IAX calculates peer ping time is using POKE/PONG/ACK sequence as follows: - Peer1 sends POKE (frame timestamp is time of sending) - Peer2 sends PONG with same time stamp received from POKE. - Peer1 calculates ping time by doing: pong_reception_time - poke_send_time. While using a shaky connection, sometimes ping time changes from normal ~100ms to long ping time (~2 seconds, sometimes get TOO LAGGED) for one ping and returns to normal again. This happens because of packet loss. Here is what happens: - Peer1 sends POKE (it mark sending time stamp in channel member `offset`) - POKE packet is dropped - After sometime (qualify / 2), POKE is retried. - Peer2 receives the POKE packet and sends PONG. - Peer1 receives the PONG packet, and calculates ping time = pong_receiption_time - first_poke_send_time So ping time becomes high for short period because of a packet retry, not because the network is slow. Is it better to calculate ping time = pong_receiption_time - last_poke_send_time? So we get **actual** network ping time instead of network_ping_time + packet_retry_time. I'd say we should always be using the time of the POKE that corresponds to the PONG that was received. Unfortunately, that doesn't appear to be easy, as I don't _think_ there's a way to know which POKE message the PONG is for - we don't send back the identity of the POKE message that caused the PONG message. (At least from what I can tell - I may be mistaken here.) The only problem with your proposal is that you may get very inaccurate qualification times. Consider a situation in which the network is terribly lagged: 1. Send POKE #1 2. Wait, get no PONG 3. Send POKE #2 4. Get a PONG message for POKE #1 With your algorithm, we'd calculate the round trip time as PONG - POKE #2 - which is very wrong. In fact, doing that may be more harmful than reporting a high qualify time due to a packet getting dropped. So I'm not sure this is the right approach - at least not without knowing how we would correlate PONGs to POKEs in such a scenario. -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated April 1, 2015, 3:33 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. more of the same for include/asterisk/dlinkedlists.h:422/431/469 main/astobj2_hash.c:768 - Maybe one of you has a better/nicer solution. Diffs (updated) - /branches/13/tests/test_linkedlists.c 433444 /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
On March 31, 2015, 8:16 p.m., rmudgett wrote: /branches/13/funcs/func_curl.c, lines 174-175 https://reviewboard.asterisk.org/r/4533/diff/7/?file=73369#file73369line174 Try defining this as: #define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) -500) This should shut-up the compiler without changing the binary value. Diederik de Groot wrote: CurlOption is a positive enum though. So the warning about it not being allowed to be negative remains, won't it ? BTW: I made a typo while changing this one I was meaning to set it to: #define CURLOPT_SPECIAL_HASHCOMPAT CURLOPT_LASTENTRY Which is a legal value. Not really happy with this though. Setting it to #define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) 500) Might be an option, not sure if this is checked though. This would require a little further source investigation. The cast should shut the compiler up as you are telling the compiler to reinterpret the -500 to be a member of the enum. If not then it's another of those compilers that tenatiously guards their stupidity. Reinterpreting a negative number as an unsigned value results in a very large number on two's complement machines. Are there any modern machines that don't use two's complement? Using CURLOPT_LASTENTRY should also work as a last resort since it has the down side of library updates adding new enum values and changing the last entry value. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/#review15003 --- On March 31, 2015, 8:59 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated March 31, 2015, 8:59 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs clang compiler warning:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:42 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- 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
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 3:16 a.m., rmudgett wrote: Thanks for your help on this one ! - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/#review15003 --- On April 1, 2015, 3:59 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 3:59 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs clang compiler warning:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 3:59 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs clang compiler warning:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
On March 31, 2015, 6:21 p.m., rmudgett wrote: /branches/13/main/utils.c, lines 492-493 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492 Revert this. This change could affect the callers of the function since you are changing the API. However, no callers currently care about the return value. You changed this because clang had a problem in test_semi1() in tests/test_strings.c. There is a better way. Diederik de Groot wrote: Actually as i tried to mention in the description i changed this according to the tests being run. I was not sure which one is supposed to be leading, the test or the source. The test says it expects: test_semi(;, , 0) To be legal. FOr it to be legal ast_alloca(0) must be allowed. Am i really changing the API here ? You are changing what is returned if buflen is zero. Before it would return outbuf while the change makes it return NULL. It is just fortunate that nothing cares about the return value. The test code is in error here because for a zero length it writes beyond the buffer boundary. On March 31, 2015, 6:21 p.m., rmudgett wrote: /branches/13/tests/test_strings.c, lines 395-396 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395 Is clang returning a NULL pointer when passed a zero length? If so this could be a problem througout the codebase since the code assumes that the function can never fail. Diederik de Groot wrote: I am not sure what happens, the compiler actually segfaults when it encountered this one. I was a little shocked about it as well. I guess the __builtins are a little more scary. If you wanna find out, give it a try. I think this should at least be reported to the llvm/clang team. I am not sure how to interpret alloca(0) either... What is the developer trying to achieve here. And what should it return. It can't allocate space of 0 length and return a pointer to it. A simple but ugly temp-fix would be to change #define ast_alloca(size) __builtin_alloca(size) to: #if defined(__clang) #define ast_alloca(size) __builtin_alloca(size ? size : 1); #else #define ast_alloca(size) __builtin_alloca(size) #endif That could be an undefined area. What is the return value of malloc(0)? Some systems return NULL while other systems return a pointer that you cannot dereference but can later pass to free(). The problem with the original test_semi() code is that with a zero length we dereference any returned pointer to put a '\0' in it. For a zero length buffer that is beyond the bounds of the buffer and will corrupt the stack. The code also reads beyond the boundary in strcmp(). - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15000 --- On March 31, 2015, 8:30 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 31, 2015, 8:30 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of
Re: [asterisk-dev] running pjsip testsuite
The following is the output [root@stnrd5652 testsuite]# ls /usr/lib/asterisk/modules | grep pjsip chan_pjsip.so func_pjsip_aor.so func_pjsip_contact.so func_pjsip_endpoint.so res_hep_pjsip.so res_pjsip_acl.so res_pjsip_authenticator_digest.so res_pjsip_caller_id.so res_pjsip_config_wizard.so res_pjsip_dialog_info_body_generator.so res_pjsip_diversion.so res_pjsip_dlg_options.so res_pjsip_dtmf_info.so res_pjsip_endpoint_identifier_anonymous.so res_pjsip_endpoint_identifier_ip.so res_pjsip_endpoint_identifier_user.so res_pjsip_exten_state.so res_pjsip_header_funcs.so res_pjsip_keepalive.so res_pjsip_log_forwarder.so res_pjsip_logger.so res_pjsip_messaging.so res_pjsip_multihomed.so res_pjsip_mwi_body_generator.so res_pjsip_mwi.so res_pjsip_nat.so res_pjsip_notify.so res_pjsip_one_touch_record_info.so res_pjsip_outbound_authenticator_digest.so res_pjsip_outbound_publish.so res_pjsip_outbound_registration.so res_pjsip_path.so res_pjsip_phoneprov_provider.so res_pjsip_pidf_body_generator.so res_pjsip_pidf_digium_body_supplement.so res_pjsip_pidf_eyebeam_body_supplement.so res_pjsip_publish_asterisk.so res_pjsip_pubsub.so res_pjsip_refer.so res_pjsip_registrar_expire.so res_pjsip_registrar.so res_pjsip_rfc3326.so res_pjsip_sdp_rtp.so res_pjsip_send_to_voicemail.so res_pjsip_session.so res_pjsip_sips_contact.so res_pjsip.so res_pjsip_t38.so res_pjsip_transport_websocket.so res_pjsip_xpidf_body_generator.so On Tue, Mar 31, 2015 at 5:39 PM, Matthew Jordan mjor...@digium.com wrote: On Tue, Mar 31, 2015 at 9:00 AM, Yaron Nachum nachum.ya...@gmail.com wrote: Thank you mathew, The pjproject was detected on the installation process. When I run Asterisk I see that pjsip modules are running. The dependency checking for Asterisk assumes that the Asterisk modules are all installed in the 'default' location: def _find_asterisk_module(self, name): Determine if an Asterisk module exists if Dependency.ast.original_astmoddir == : return False module = %s/%s.so % (Dependency.ast.original_astmoddir, name) if os.path.exists(module): return True return False The fact that this is finding some of your Asterisk modules (app_echo) but not the PJSIP ones is a bit odd. What is the output of: ls /usr/lib/asterisk/modules | grep pjsip -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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 -- _ -- 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] 4557: Tell menuselect that MALLOC_DEBUG conflicts with DEBUG_CHAOS.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4557/#review14969 --- Ship it! Ship It! - Joshua Colp On March 29, 2015, 6:30 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4557/ --- (Updated March 29, 2015, 6:30 p.m.) Review request for Asterisk Developers and Scott Griepentrog. Repository: Asterisk Description --- DEBUG_CHAOS is marked as conflicting with MALLOC_DEBUG, but for this to work correctly MALLOC_DEBUG must also be marked as conflicting with DEBUG_CHAOS. Diffs - /branches/13/build_tools/cflags.xml 433716 Diff: https://reviewboard.asterisk.org/r/4557/diff/ Testing --- make menuselect, verified that enabling either option disabled the other. 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] 4563: chan_sip: handle IPv4 mapped clients when NAT and IPv6 socket is enabled
On March 31, 2015, 3:19 p.m., Michael Young wrote: I would hold off on committing this patch as I feel it is not really a proper solution. I believe that this is just short circuiting the conditional and always applying remapping if IPv6 is involved or not. On the issue tracker, Valentin has provided some debug logs which, unless I am reading them wrong, point to a configuration error. The IPv4 addresses are not being mapped to IPv6 at all according to the debug logs. If the socket is bound to :: they'll be IPv6 mapped. I've seen this in the PJSIP world (where it's not supported). - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/#review14973 --- On March 30, 2015, 2:23 p.m., Valentin Vidić wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/ --- (Updated March 30, 2015, 2:23 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-18032 https://issues.asterisk.org/jira/browse/ASTERISK-18032 Repository: Asterisk Description --- When udpbindaddr=:: is set Asterisk accepts IPv4 and IPv6 clients both stored in a struct sockaddr_in6 with AF_INET6 family type. Current NAT code for IPv4 checks if the socket type is AF_INET6 and thus fails to handle IPv4 mapped addresses properly. The patch adds an additional check for this case allowing IPv4 clients to be handled by NAT even when IPv6 is enabled. Diffs - /branches/11/channels/chan_sip.c 433794 Diff: https://reviewboard.asterisk.org/r/4563/diff/ Testing --- Patch solves the problem of failing incoming calls on a local NATed installation with IPv6 sockets enabled. Thanks, Valentin Vidić -- _ -- 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] running pjsip testsuite
Got it !!! The testsuite was looking for these modules in /usr/lib64. I recompiled the asterisk with --libdir=/usr/lib64 and it works. Now the test is running I will start working on it now. Thank you. On Tue, Mar 31, 2015 at 6:09 PM, Yaron Nachum nachum.ya...@gmail.com wrote: The following is the output [root@stnrd5652 testsuite]# ls /usr/lib/asterisk/modules | grep pjsip chan_pjsip.so func_pjsip_aor.so func_pjsip_contact.so func_pjsip_endpoint.so res_hep_pjsip.so res_pjsip_acl.so res_pjsip_authenticator_digest.so res_pjsip_caller_id.so res_pjsip_config_wizard.so res_pjsip_dialog_info_body_generator.so res_pjsip_diversion.so res_pjsip_dlg_options.so res_pjsip_dtmf_info.so res_pjsip_endpoint_identifier_anonymous.so res_pjsip_endpoint_identifier_ip.so res_pjsip_endpoint_identifier_user.so res_pjsip_exten_state.so res_pjsip_header_funcs.so res_pjsip_keepalive.so res_pjsip_log_forwarder.so res_pjsip_logger.so res_pjsip_messaging.so res_pjsip_multihomed.so res_pjsip_mwi_body_generator.so res_pjsip_mwi.so res_pjsip_nat.so res_pjsip_notify.so res_pjsip_one_touch_record_info.so res_pjsip_outbound_authenticator_digest.so res_pjsip_outbound_publish.so res_pjsip_outbound_registration.so res_pjsip_path.so res_pjsip_phoneprov_provider.so res_pjsip_pidf_body_generator.so res_pjsip_pidf_digium_body_supplement.so res_pjsip_pidf_eyebeam_body_supplement.so res_pjsip_publish_asterisk.so res_pjsip_pubsub.so res_pjsip_refer.so res_pjsip_registrar_expire.so res_pjsip_registrar.so res_pjsip_rfc3326.so res_pjsip_sdp_rtp.so res_pjsip_send_to_voicemail.so res_pjsip_session.so res_pjsip_sips_contact.so res_pjsip.so res_pjsip_t38.so res_pjsip_transport_websocket.so res_pjsip_xpidf_body_generator.so On Tue, Mar 31, 2015 at 5:39 PM, Matthew Jordan mjor...@digium.com wrote: On Tue, Mar 31, 2015 at 9:00 AM, Yaron Nachum nachum.ya...@gmail.com wrote: Thank you mathew, The pjproject was detected on the installation process. When I run Asterisk I see that pjsip modules are running. The dependency checking for Asterisk assumes that the Asterisk modules are all installed in the 'default' location: def _find_asterisk_module(self, name): Determine if an Asterisk module exists if Dependency.ast.original_astmoddir == : return False module = %s/%s.so % (Dependency.ast.original_astmoddir, name) if os.path.exists(module): return True return False The fact that this is finding some of your Asterisk modules (app_echo) but not the PJSIP ones is a bit odd. What is the output of: ls /usr/lib/asterisk/modules | grep pjsip -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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 -- _ -- 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] running pjsip testsuite
Another thing that is important is that the sample configs must be installed. Many tests have some difficulty if this is not the case. For me it was because I had configurations defining the same endpoints with chan_sip and chan_pjsip. The conflicting configs caused crashes in tests that did not use SIP at all. Richard -- _ -- 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] 4542: DNS: Add NAPTR support and tests
On March 31, 2015, 10:25 a.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 447-449
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line447
Suggestion: since this is repeated after each check, you may want to
macro-tize it:
#define CHECK_END_OF_RECORD do { \
if (ptr = end_of_record) { \
return NULL; \
} } while (0)
Then you can just put:
ptr += 2;
CHECK_END_OF_RECORD;
Or something like that.
Doing this hides return points. Which adds a potential for memory and ref
leaks.
- rmudgett
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
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] 4563: chan_sip: handle IPv4 mapped clients when NAT and IPv6 socket is enabled
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/#review14973 --- I would hold off on committing this patch as I feel it is not really a proper solution. I believe that this is just short circuiting the conditional and always applying remapping if IPv6 is involved or not. On the issue tracker, Valentin has provided some debug logs which, unless I am reading them wrong, point to a configuration error. The IPv4 addresses are not being mapped to IPv6 at all according to the debug logs. - Michael Young On March 30, 2015, 10:23 a.m., Valentin Vidić wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/ --- (Updated March 30, 2015, 10:23 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-18032 https://issues.asterisk.org/jira/browse/ASTERISK-18032 Repository: Asterisk Description --- When udpbindaddr=:: is set Asterisk accepts IPv4 and IPv6 clients both stored in a struct sockaddr_in6 with AF_INET6 family type. Current NAT code for IPv4 checks if the socket type is AF_INET6 and thus fails to handle IPv4 mapped addresses properly. The patch adds an additional check for this case allowing IPv4 clients to be handled by NAT even when IPv6 is enabled. Diffs - /branches/11/channels/chan_sip.c 433794 Diff: https://reviewboard.asterisk.org/r/4563/diff/ Testing --- Patch solves the problem of failing incoming calls on a local NATed installation with IPv6 sockets enabled. Thanks, Valentin Vidić -- _ -- 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] running pjsip testsuite
Thank you mathew, The pjproject was detected on the installation process. When I run Asterisk I see that pjsip modules are running. Any idea? Yaron On Tue, Mar 31, 2015 at 4:12 PM, Matthew Jordan mjor...@digium.com wrote: On Tue, Mar 31, 2015 at 8:04 AM, Yaron Nachum nachum.ya...@gmail.com wrote: Hi everyone, I am trying to add some tests for the PJSIP auto-dtmf support . Before I start I just wanted to run some of the existing tests in order to understand the process. Whenever I try to run a test from the pjsip tests I get - --- -- Dependency: res_pjsip - False. The following output was generated when I tried to run the rpid_immediate test: [root@stnrd5652 testsuite]# ./runtests.py -t tests/channels/pjsip/rpid_immediate/ Running tests for Asterisk SVN-trunk-r431522M ... Tests to run: 0, Maximum test inactivity time: -1 sec. -- Cannot run test 'tests/channels/pjsip/rpid_immediate' --- -- Minimum Version: 13.4.0 (True) --- -- Maximum Version: (True) --- -- Tags: ['pjsip'] --- -- Dependency: twisted - True --- -- Dependency: starpy - True --- -- Dependency: app_dial - True --- -- Dependency: app_echo - True --- -- Dependency: func_callerid - True --- -- Dependency: chan_pjsip - False --- -- Dependency: res_pjsip - False --- -- Dependency: res_pjsip_caller_id - False --- -- Dependency: res_pjsip_endpoint_identifier_user - False --- -- Dependency: res_pjsip_sdp_rtp - False --- -- Dependency: res_pjsip_session - False ?xml version=1.0 encoding=utf-8? testsuite errors=0 failures=0 name=AsteriskTestSuite tests=0 time=0.00/ Am I doing anything stupid? Congratulations on getting this far! Looks like you've got most of the dependencies worked out in the testsuite. The dependency checking for 'res_pjsip' is looking at what modules you have installed on the system. Double check that your Asterisk installation did detect pjproject, and that it built and installed the res_pjsip* modules. You can check this in menuselect. Matt -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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 -- _ -- 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] 4563: chan_sip: handle IPv4 mapped clients when NAT and IPv6 socket is enabled
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/#review14970 --- Ship it! Ship It! - Joshua Colp On March 30, 2015, 2:23 p.m., Valentin Vidić wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/ --- (Updated March 30, 2015, 2:23 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-18032 https://issues.asterisk.org/jira/browse/ASTERISK-18032 Repository: Asterisk Description --- When udpbindaddr=:: is set Asterisk accepts IPv4 and IPv6 clients both stored in a struct sockaddr_in6 with AF_INET6 family type. Current NAT code for IPv4 checks if the socket type is AF_INET6 and thus fails to handle IPv4 mapped addresses properly. The patch adds an additional check for this case allowing IPv4 clients to be handled by NAT even when IPv6 is enabled. Diffs - /branches/11/channels/chan_sip.c 433794 Diff: https://reviewboard.asterisk.org/r/4563/diff/ Testing --- Patch solves the problem of failing incoming calls on a local NATed installation with IPv6 sockets enabled. Thanks, Valentin Vidić -- _ -- 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] 4563: chan_sip: handle IPv4 mapped clients when NAT and IPv6 socket is enabled
On March 31, 2015, 3:19 p.m., Michael Young wrote: I would hold off on committing this patch as I feel it is not really a proper solution. I believe that this is just short circuiting the conditional and always applying remapping if IPv6 is involved or not. On the issue tracker, Valentin has provided some debug logs which, unless I am reading them wrong, point to a configuration error. The IPv4 addresses are not being mapped to IPv6 at all according to the debug logs. Joshua Colp wrote: If the socket is bound to :: they'll be IPv6 mapped. I've seen this in the PJSIP world (where it's not supported). To respond to myself: I also checked the ACL code which is used for the localnet stuff, and it seems to support IPv6 mapped addresses. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/#review14973 --- On March 30, 2015, 2:23 p.m., Valentin Vidić wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4563/ --- (Updated March 30, 2015, 2:23 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-18032 https://issues.asterisk.org/jira/browse/ASTERISK-18032 Repository: Asterisk Description --- When udpbindaddr=:: is set Asterisk accepts IPv4 and IPv6 clients both stored in a struct sockaddr_in6 with AF_INET6 family type. Current NAT code for IPv4 checks if the socket type is AF_INET6 and thus fails to handle IPv4 mapped addresses properly. The patch adds an additional check for this case allowing IPv4 clients to be handled by NAT even when IPv6 is enabled. Diffs - /branches/11/channels/chan_sip.c 433794 Diff: https://reviewboard.asterisk.org/r/4563/diff/ Testing --- Patch solves the problem of failing incoming calls on a local NATed installation with IPv6 sockets enabled. Thanks, Valentin Vidić -- _ -- 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] running pjsip testsuite
On Tue, Mar 31, 2015 at 9:00 AM, Yaron Nachum nachum.ya...@gmail.com wrote: Thank you mathew, The pjproject was detected on the installation process. When I run Asterisk I see that pjsip modules are running. The dependency checking for Asterisk assumes that the Asterisk modules are all installed in the 'default' location: def _find_asterisk_module(self, name): Determine if an Asterisk module exists if Dependency.ast.original_astmoddir == : return False module = %s/%s.so % (Dependency.ast.original_astmoddir, name) if os.path.exists(module): return True return False The fact that this is finding some of your Asterisk modules (app_echo) but not the PJSIP ones is a bit odd. What is the output of: ls /usr/lib/asterisk/modules | grep pjsip -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] running pjsip testsuite
On Tue, Mar 31, 2015 at 10:29 AM, Richard Mudgett rmudg...@digium.com wrote: Another thing that is important is that the sample configs must be installed. Many tests have some difficulty if this is not the case. For me it was because I had configurations defining the same endpoints with chan_sip and chan_pjsip. The conflicting configs caused crashes in tests that did not use SIP at all. *Most* of that has been resolved now, thanks to the 'is this test using chan_sip or chan_pjsip' logic added by Kevin. But generally, yes, using 'make samples' - or having enough configuration installed to get Asterisk up and running - is needed. -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] 4528: dns: Add SRV recording parsing, sorting/weighting, and unit tests
On March 31, 2015, 10:28 a.m., Mark Michelson wrote: /trunk/main/dns_srv.c, lines 98-100 https://reviewboard.asterisk.org/r/4528/diff/2/?file=72947#file72947line98 Let's talk alignment. In DNS, there is no padding used for variable-length elements. There is no guarantee that RRs (or their RDATA) lie on word-boundaries. This means that the data pointer passed to ast_dns_srv_alloc() may be pointing to data that is not word-aligned (or not even on a half-word). So in this case, you've overlaid a byte array on top of a struct that contains 16-bit unsigned integers. Typically, 16-bit values should be aligned on word or half-word boundaries, but because of the data layout of the DNS record, this cannot be guaranteed. This means you may be performing some unaligned reads here. Now the question is, how bad is this? I'm pretty sure that on x86 architectures, an unaligned read isn't a big deal, performance-wise, but I can't make any claims for other architectures simply because I don't know. Can someone more familiar with these sorts of things chime in with some info? For x86 platforms, misaligned values are handled without a problem but with a performance penalty. On other processors such as the 68000 you get an alignment exception. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/#review14971 --- On March 26, 2015, 1:50 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/ --- (Updated March 26, 2015, 1:50 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This change adds the following: 1. As SRV records are added to a result the information is parsed and stored away in additional storage in the record. The SRV API can then be used to return this information. 2. Before invoking the DNS query callback the list of records on the result are sorted based on priority and weight. 3. Unit tests have been added which verify the record parsing, sorting, and weighting. There are also some off nominal which cover the cases when an invalid/corrupt record is received. 4. A unit test has also been added to res_resolver_unbound which adds an SRV record to a zone and confirms it is retrieved and parsed correctly. Diffs - /trunk/tests/test_dns_srv.c PRE-CREATION /trunk/res/res_resolver_unbound.c 433370 /trunk/main/dns_srv.c 433370 /trunk/main/dns_core.c 433370 /trunk/include/asterisk/dns_internal.h 433370 Diff: https://reviewboard.asterisk.org/r/4528/diff/ Testing --- Executed unit tests and confirmed they pass. 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] 4520: Testsuite: stasis: set a channel variable on websocket disconnect error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4520/ --- (Updated March 31, 2015, 10:58 a.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24802 https://issues.asterisk.org/jira/browse/ASTERISK-24802 Repository: testsuite Description --- This review has been moved to gerrit: https://gerrit.asterisk.org/#/c/18/ When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis). This patch introduces a new channel variable: STASISSTATUS to give outside applications context when errors occur in Stasis that interrupt normal processing. This test exercises three scenarios to elicit updates to the STASISSTATUS channel variable: 1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis correctly assigns SUCCESS to STASISSTATUS. 2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied. 3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied. ***Note*** This is a test. It is only a test. The review for the Asterisk source can be found at: https://reviewboard.asterisk.org/r/4519/ Diffs - ./asterisk/trunk/tests/rest_api/applications/tests.yaml 6547 ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario_factory.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_case.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test-config.yaml PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/run-test PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/observable_object.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/monitor.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/sip.conf PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/extensions.conf PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/ari_client.py PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4520/diff/ Testing --- Thanks, Ashley Sanders -- _ -- 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] 4541: clang compiler warning: -Wpointer-bool-conversion
On March 31, 2015, 5:50 p.m., Mark Michelson wrote:
/branches/13/apps/app_queue.c, lines 9089-9093
https://reviewboard.asterisk.org/r/4541/diff/1/?file=73002#file73002line9089
This change is not correct. While the mem-state_interface will always
be non-NULL (thanks, clang!), it may not have been configured and will
therefore be zero-length.
The correct fix here is to switch the previous
if (mem-state_interface)
to be
if (ast_strlen_zero(mem-state_interface))
Thanks for pointing that one out.
Fixed using if (!ast_strlen_zero(mem-state_interface)) instead :-)
On March 31, 2015, 5:50 p.m., Mark Michelson wrote:
/branches/13/res/res_smdi.c, lines 876-881
https://reviewboard.asterisk.org/r/4541/diff/1/?file=73004#file73004line876
This change is incorrect. The if checks should be changed to
if (ast_strlen_zero(search_msg-mesg_dsk_num))
and
if (ast_strlen_zero(search_msg-mesg_desk_term))
Fixed using if (!ast_strlen_zero(...)) instead.
Thanks Mark !
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review14978
---
On March 31, 2015, 6:05 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 6:05 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] 4520: Testsuite: stasis: set a channel variable on websocket disconnect error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4520/ --- (Updated March 31, 2015, 10:58 a.m.) Review request for Asterisk Developers. Changes --- Noted in the description that the review has been moved to gerrit. Bugs: ASTERISK-24802 https://issues.asterisk.org/jira/browse/ASTERISK-24802 Repository: testsuite Description (updated) --- This review has been moved to gerrit: https://gerrit.asterisk.org/#/c/18/ When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis). This patch introduces a new channel variable: STASISSTATUS to give outside applications context when errors occur in Stasis that interrupt normal processing. This test exercises three scenarios to elicit updates to the STASISSTATUS channel variable: 1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis correctly assigns SUCCESS to STASISSTATUS. 2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied. 3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied. ***Note*** This is a test. It is only a test. The review for the Asterisk source can be found at: https://reviewboard.asterisk.org/r/4519/ Diffs - ./asterisk/trunk/tests/rest_api/applications/tests.yaml 6547 ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario_factory.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_case.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test-config.yaml PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/run-test PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/observable_object.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/monitor.py PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/sip.conf PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/extensions.conf PRE-CREATION ./asterisk/trunk/tests/rest_api/applications/stasisstatus/ari_client.py PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4520/diff/ Testing --- Thanks, Ashley Sanders -- _ -- 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] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14980 --- Ship it! Ship It! - Mark Michelson On March 28, 2015, 3:08 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 28, 2015, 3:08 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4528: dns: Add SRV recording parsing, sorting/weighting, and unit tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/#review14981 --- Ship it! Ship It! - Mark Michelson On March 31, 2015, 3:50 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/ --- (Updated March 31, 2015, 3:50 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This change adds the following: 1. As SRV records are added to a result the information is parsed and stored away in additional storage in the record. The SRV API can then be used to return this information. 2. Before invoking the DNS query callback the list of records on the result are sorted based on priority and weight. 3. Unit tests have been added which verify the record parsing, sorting, and weighting. There are also some off nominal which cover the cases when an invalid/corrupt record is received. 4. A unit test has also been added to res_resolver_unbound which adds an SRV record to a zone and confirms it is retrieved and parsed correctly. Diffs - /trunk/tests/test_dns_srv.c PRE-CREATION /trunk/res/res_resolver_unbound.c 433815 /trunk/main/dns_srv.c 433815 /trunk/main/dns_core.c 433815 /trunk/include/asterisk/dns_internal.h 433815 Diff: https://reviewboard.asterisk.org/r/4528/diff/ Testing --- Executed unit tests and confirmed they pass. 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] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 6:05 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs (updated)
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/#review14984 --- /team/group/dns/include/asterisk/dns_internal.h https://reviewboard.asterisk.org/r/4542/#comment25605 These methods should not start with ast_* unless they are meant to be exposed externally/exported. If they are then they should be moved to a more public include file. /team/group/dns/main/dns_naptr.c https://reviewboard.asterisk.org/r/4542/#comment25604 This seems like it should be a non assert check. What happens if asterisk is not compiled without debug on and this is false? /team/group/dns/res/res_resolver_unbound.c https://reviewboard.asterisk.org/r/4542/#comment25608 Any reason to continue if a failure occurs? /team/group/dns/tests/test_dns_naptr.c https://reviewboard.asterisk.org/r/4542/#comment25606 Should these failures break the loop and just goto cleanup as well? - Kevin Harwell On March 27, 2015, 9:45 a.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated March 27, 2015, 9:45 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433573 /team/group/dns/main/dns_naptr.c 433573 /team/group/dns/main/dns_core.c 433573 /team/group/dns/include/asterisk/dns_internal.h 433573 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. 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] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review14985
---
Ship it!
There's only one finding left and it's just to swap two lines of code, so I'm
giving a ship it!
/branches/13/apps/app_minivm.c
https://reviewboard.asterisk.org/r/4541/#comment25609
The if check here should be before the ast_copy_string() operation.
Basically, setting oldlocale to something in the case that etemplate-locale is
zero-length is a wasted operation since we will not end up doing anything with
oldlocale in that case.
- Mark Michelson
On March 31, 2015, 4:05 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 4:05 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] 4553: clang compiler warning: dev-mode and -Wunused-function
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4553/#review14983 --- Ship it! Ship It! - Mark Michelson On March 28, 2015, 6:22 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4553/ --- (Updated March 28, 2015, 6:22 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:dev-mode and -Wunused-function Diffs - /branches/13/utils/extconf.c 433444 /branches/13/res/parking/parking_tests.c 433444 Diff: https://reviewboard.asterisk.org/r/4553/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4528: dns: Add SRV recording parsing, sorting/weighting, and unit tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/ --- (Updated March 31, 2015, 3:50 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This change adds the following: 1. As SRV records are added to a result the information is parsed and stored away in additional storage in the record. The SRV API can then be used to return this information. 2. Before invoking the DNS query callback the list of records on the result are sorted based on priority and weight. 3. Unit tests have been added which verify the record parsing, sorting, and weighting. There are also some off nominal which cover the cases when an invalid/corrupt record is received. 4. A unit test has also been added to res_resolver_unbound which adds an SRV record to a zone and confirms it is retrieved and parsed correctly. Diffs (updated) - /trunk/tests/test_dns_srv.c PRE-CREATION /trunk/res/res_resolver_unbound.c 433815 /trunk/main/dns_srv.c 433815 /trunk/main/dns_core.c 433815 /trunk/include/asterisk/dns_internal.h 433815 Diff: https://reviewboard.asterisk.org/r/4528/diff/ Testing --- Executed unit tests and confirmed they pass. 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] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review14978
---
/branches/13/apps/app_minivm.c
https://reviewboard.asterisk.org/r/4541/#comment25600
This should be
if (ast_strlen_zero(etemplate-locale))
/branches/13/apps/app_minivm.c
https://reviewboard.asterisk.org/r/4541/#comment25601
This should be
if (ast_strlen_zero(etemplate-locale))
/branches/13/apps/app_queue.c
https://reviewboard.asterisk.org/r/4541/#comment25597
This change is not correct. While the mem-state_interface will always be
non-NULL (thanks, clang!), it may not have been configured and will therefore
be zero-length.
The correct fix here is to switch the previous
if (mem-state_interface)
to be
if (ast_strlen_zero(mem-state_interface))
/branches/13/res/res_smdi.c
https://reviewboard.asterisk.org/r/4541/#comment25599
This change is incorrect. The if checks should be changed to
if (ast_strlen_zero(search_msg-mesg_dsk_num))
and
if (ast_strlen_zero(search_msg-mesg_desk_term))
- Mark Michelson
On March 27, 2015, 12:13 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 27, 2015, 12:13 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] RFC: Refactor qualify and res_pjsip/endpt_send_request
On 03/31/2015 01:51 AM, Olle E. Johansson wrote: On 30 Mar 2015, at 16:54, Mark Michelson mmichel...@digium.com wrote: On 03/28/2015 08:06 PM, Joshua Colp wrote: George Joseph wrote: The fact that it goes to unavailable would be a bug. Why does it do so? Mark should probably chime in here but I think it's because the earliest you could get a response from pjsip when a contact isn't reachable is the unconfigurable 32 seconds. As I said that's a long time to leave a contact available when it really isn't. Without implementing our own timer setting the contact to unavailable was probably the lesser of 2 evils. No matter what there's going to be a period where you are potentially wrong. I don't think making it unavailable was done on purpose. Actually, I believe the timer may be configurable. In the type=system settings, there are timer_t1 and timer_b settings. timer_t1 is the base used for determining the retransmission interval, and timer_b is the maximum time we will wait before giving up sending the request. The defaults for these values are 500 ms and 32000 ms respectively. If you were to change timer_b to be a smaller value, then presumably you would have a shorter time before the transaction times out. A couple of caveats about these settings 1) Since they're in the type=system settings, any change you make requires an Asterisk restart in order to take effect. Are you serious? That's a very strange design. Without knowing anything about PJSIP, I think that is something that needs to be fixed. There are several SIP phones based on PJSIP where I can set timers without restarting. Wonder how they did it. The issue is that the configured timer values are copied by the PJSIP transaction layer when the transaction layer is initialized. Any changes made to the configured timer values are not taken into account by the transaction layer since it never re-reads the configured values. If you were able to change timers, then either 1) The implementors were using a much lower level of the PJSIP API in order to manage timers themselves. 2) They had made changes to the PJSIP source code to allow the transaction layer to re-learn configured timer values. 3) They were stopping and restarting PJSIP under the hood each time that the timer values were changed. As far as requiring an Asterisk restart goes, I suppose that's not actually true anymore. I had forgotten that we had made res_pjsip.so unloadable now. You could actually unload res_pjsip.so and then load it again to apply type=system changes on a running Asterisk system. It's still not pleasant, but it's better than a full-scale restart. 2) PJSIP applies these timers globally. They will affect ALL SIP transactions, not just the OPTIONS transactions from the qualify checks. That seems very strange, but I have to trust you here. It makes the channel driver rather unusable in gateway situations where it's a requirement that I have one set of timers for my internal systems and one for external clients. /O -- _ -- 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] 4542: DNS: Add NAPTR support and tests
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
/team/group/dns/include/asterisk/dns_internal.h
https://reviewboard.asterisk.org/r/4542/#comment25588
Doxygen comment.
In particular, the comment should explain the relationship of data_ptr to
data, and why it is necessary.
/team/group/dns/include/asterisk/dns_internal.h
https://reviewboard.asterisk.org/r/4542/#comment25589
Doxygen comment
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25591
The asserts here are appropriate.
However, if there is an error in the record, such that the memcmp never
returns true, we'll get stuck in this loop.
It may be good to have a 'fail safe' break out in the loop, after the
asserts. That way in dev-mode we'll catch issues, but in production systems,
the allocation of the NAPTR record will fail and we can (hopefully) gracefully
handle it.
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25592
Suggestion: since this is repeated after each check, you may want to
macro-tize it:
#define CHECK_END_OF_RECORD do { \
if (ptr = end_of_record) { \
return NULL; \
} } while (0)
Then you can just put:
ptr += 2;
CHECK_END_OF_RECORD;
Or something like that.
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25590
I'd check to make sure num_records is non-zero before allocating the array.
If it is zero, you can simply bail out of the routine.
- Matt Jordan
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 12:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
Diederik de Groot wrote:
@rmudgett: I agreed, that's why i was asking in my initial description,
if anybody knew of a better way to solve both the macro issue but still
satisfy the parantheses warning. Without having to suppress it. Something
ugly to fix the macro expansion is to use a double negating comparison or
maybe even an inline comparison return TRUE/FALSE. Both of which did not
really strike me as particularly nice.
I can live with suppressing this warning, but it would be nice if
somebody knew of a nice way to have it both ways.
Please discard this review with predjudice. I see no benefit to this
-Wparentheses-equality option.
- rmudgett
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 12:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 12:14 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
Because of the () where removed previously, they are now needed here.
main/stasis_message_router.c:82
===
Removed the extraneous (). Not particularly happy about this
Re: [asterisk-dev] Asterisk to Git Migration Update
On Tue, Mar 31, 2015 at 2:32 PM, Matthew Jordan mjor...@digium.com wrote: Hey everyone: Hi! Just as a brief status update on the Asterisk Git Migration project, here is where we stand today, and what the next steps are: Awesome work so far. :-) There's a large amount of intervening work that has to be done, and some questions that still need to be answered. In no particular order: (1) In test runs of building the Asterisk repo, I've successfully managed to pull in the team repos that are still 'active'. Since Gerrit acts as the canonical Git repo, we can set it up so that direct pushes to team branches are allowed and don't require a code review - although code reviews can be done if someone desires it. For those who use team branches a lot, does this sound acceptable? What's the benefit of trying to keep team branches at all vs. just letting people use their own git trees hosted on one of the many personal git hosting options (github or whatever) ? If it's about several people collaborating on a branch, that makes sense to me to do as a feature branch in gerrit, but it seems like the exception, not the rule. (2) Today, we merge up the branches - 11 = 13, 13 = trunk, etc. After the move, it looks like the best way to handle the merge process is going to be to make patches against trunk, then cherry-pick them back to the currently maintained branches. For long time users of Gerrit, does this seem appropriate? Yes. (3) In Asterisk 13, we pulled in menuselect (yay). On the downside, Asterisk 11 doesn't have menuselect. What's more, Asterisk 1.8 and 12 are still in security fix mode. We really have two options here: (a) Update the build scripts to pull in menuselect as they do now from SVN, and more or less keep the existing process. My fear is that this may turn into a bit of hackery to keep the Git/SVN lines from getting confused. (b) Bite the bullet and just backport menuselect into all the branches. Moving to Git is going to be a breaking change for people using Asterisk 11 anyway, and this way things end up in a reasonably pristine state. Thoughts? (b) sounds the least painful overall to me. People consuming releases shouldn't notice a difference, right? (4) Asterisk records the SVN revision in each file using the special keyword $Revision:. This is then registered in a linked list for retrieval by the CLI/AMI. Unfortunately, Git doesn't support this concept, as adding data into a file after commit would change the checksum . It does allow doing this on checkout via the $Id$ keyword, which may be an acceptable workaround. That whole thing seems questionably useful, anyway. Just removing it is another option. Anyway, the goal is to kick off the move of Asterisk to Git immediately after we get the next batch of releases out. I'll send out an e-mail once we know exactly when that is. \o/ -- Russell Bryant -- _ -- 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] 4549: ARI: Add the ability to intercept hold and raise an event
On March 31, 2015, 12:23 p.m., Mark Michelson wrote: One thing to take into consideration here is that there are some places within Asterisk where we will send an AST_CONTROL_UNHOLD frame on a channel, even though it may not currently be on hold. This means you may send some unhold ARI events that don't match up with a previous hold event. This is probably worth documenting somewhere so that ARI application writers know what they might have to deal with. In what circumstances do we do that? - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/#review14988 --- On March 27, 2015, 10:19 p.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/ --- (Updated March 27, 2015, 10:19 p.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24922 https://issues.asterisk.org/jira/browse/ASTERISK-24922 Repository: Asterisk Description --- For some applications - such as SLA - a phone pressing hold should not behave in the fashion that the Asterisk core would like it to. Instead, the hold action has some application specific behaviour associated with it - such as disconnecting the channel that initiated the hold; only playing MoH to channels in the bridge if the channels are of a particular type, etc. One way of accomplishing this is to use a framehook to intercept the hold/unhold frames, raise an event, and eat the frame. Tasty. The patch attached to this issue accomplished that as a new dialplan function, HOLD_INTERCEPT. In addition: * ARI now queues hold/unhold frames instead of indicating frames directly. This allows for the Stasis hold/unhold messages to be raised. * Some general cleanup of raising hold/unhold Stasis messages was done, including removing some RAII_VAR usage. Diffs - /branches/13/rest-api/api-docs/events.json 433677 /branches/13/res/stasis/control.c 433677 /branches/13/res/stasis/app.c 433677 /branches/13/res/ari/ari_model_validators.c 433677 /branches/13/res/ari/ari_model_validators.h 433677 /branches/13/main/stasis_channels.c 433677 /branches/13/main/manager_channels.c 433677 /branches/13/main/channel.c 433677 /branches/13/main/bridge_channel.c 433677 /branches/13/funcs/func_holdintercept.c PRE-CREATION /branches/13/CHANGES 433677 Diff: https://reviewboard.asterisk.org/r/4549/diff/ Testing --- See Gerrit reviews: https://gerrit.asterisk.org/#/c/16 https://gerrit.asterisk.org/#/c/17 Thanks, Matt Jordan -- _ -- 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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
@rmudgett: I agreed, that's why i was asking in my initial description, if
anybody knew of a better way to solve both the macro issue but still satisfy
the parantheses warning. Without having to suppress it. Something ugly to fix
the macro expansion is to use a double negating comparison or maybe even an
inline comparison return TRUE/FALSE. Both of which did not really strike me as
particularly nice.
I can live with suppressing this warning, but it would be nice if somebody knew
of a nice way to have it both ways.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
Because of the () where removed previously, they are now needed here.
main/stasis_message_router.c:82
===
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double
Re: [asterisk-dev] [Code Review] 4549: ARI: Add the ability to intercept hold and raise an event
On March 31, 2015, 5:23 p.m., Mark Michelson wrote: One thing to take into consideration here is that there are some places within Asterisk where we will send an AST_CONTROL_UNHOLD frame on a channel, even though it may not currently be on hold. This means you may send some unhold ARI events that don't match up with a previous hold event. This is probably worth documenting somewhere so that ARI application writers know what they might have to deal with. Matt Jordan wrote: In what circumstances do we do that? I was thinking of transfer code in particular. The transfer code does not know whether the channel being transferred is on hold or not, but it will send an unhold frame anyway. - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/#review14988 --- On March 28, 2015, 3:19 a.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/ --- (Updated March 28, 2015, 3:19 a.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24922 https://issues.asterisk.org/jira/browse/ASTERISK-24922 Repository: Asterisk Description --- For some applications - such as SLA - a phone pressing hold should not behave in the fashion that the Asterisk core would like it to. Instead, the hold action has some application specific behaviour associated with it - such as disconnecting the channel that initiated the hold; only playing MoH to channels in the bridge if the channels are of a particular type, etc. One way of accomplishing this is to use a framehook to intercept the hold/unhold frames, raise an event, and eat the frame. Tasty. The patch attached to this issue accomplished that as a new dialplan function, HOLD_INTERCEPT. In addition: * ARI now queues hold/unhold frames instead of indicating frames directly. This allows for the Stasis hold/unhold messages to be raised. * Some general cleanup of raising hold/unhold Stasis messages was done, including removing some RAII_VAR usage. Diffs - /branches/13/rest-api/api-docs/events.json 433677 /branches/13/res/stasis/control.c 433677 /branches/13/res/stasis/app.c 433677 /branches/13/res/ari/ari_model_validators.c 433677 /branches/13/res/ari/ari_model_validators.h 433677 /branches/13/main/stasis_channels.c 433677 /branches/13/main/manager_channels.c 433677 /branches/13/main/channel.c 433677 /branches/13/main/bridge_channel.c 433677 /branches/13/funcs/func_holdintercept.c PRE-CREATION /branches/13/CHANGES 433677 Diff: https://reviewboard.asterisk.org/r/4549/diff/ Testing --- See Gerrit reviews: https://gerrit.asterisk.org/#/c/16 https://gerrit.asterisk.org/#/c/17 Thanks, Matt Jordan -- _ -- 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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
Diederik de Groot wrote:
@rmudgett: I agreed, that's why i was asking in my initial description,
if anybody knew of a better way to solve both the macro issue but still
satisfy the parantheses warning. Without having to suppress it. Something
ugly to fix the macro expansion is to use a double negating comparison or
maybe even an inline comparison return TRUE/FALSE. Both of which did not
really strike me as particularly nice.
I can live with suppressing this warning, but it would be nice if
somebody knew of a nice way to have it both ways.
rmudgett wrote:
Please discard this review with predjudice. I see no benefit to this
-Wparentheses-equality option.
WOuld have been nice if this Warning would not react to results coming from
macro's (so only plain code), but alas. I guess the AST does not have that last
little detail.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
[asterisk-dev] Change in testsuite[master]: Rewrite sip_attended_transfer test to stop failing.
Ashley Sanders has posted comments on this change. Change subject: Rewrite sip_attended_transfer test to stop failing. .. Patch Set 2: Code-Review-1 (6 comments) Overall, this is pretty good. The flow was mostly easy to follow. I only have a few findings with respect to some opportunities to use abstractions (that could help reduce the complexity of working around the caveats of the pjsua api). https://gerrit.asterisk.org/#/c/19/2/tests/channels/SIP/sip_attended_transfer/attended_transfer.py File tests/channels/SIP/sip_attended_transfer/attended_transfer.py: Line 36: class BobCallCallback(pj.CallCallback): Just as something to mention, BobCallCallback and CarolCallCallback can be reduced into one class, with the function to callback as a ctor parameter. The transfer object itself is not needed. Line 86: self.bridge1 = None Another fyi - The bridge1 and bridge2 could be made into a class and you could have a attributes on the class, bridge_id and 'active' or 'is_bridged' to hold the state of the object, rather than having 2 variables for each bridge. I can see this pattern as being a little bit cleaner and easier to read if anyone ever has to come back and revisit this in the future. Line 121: self.call_carol() I think that you are calling this twice, once in the BobCallCallback.on_state handler (ln 48) callback and here again, on the ami.bridge_enter handler. Line 132: self.transfer_call() Here, too, this seems to be called twice; once in the CarolCallCallback.on_state handler (ln 69) and again here, in the ami.bridge_enter handler. Line 154: if (self.state == BOB_CALLED and self.bridge1_bridged and I don't think you need the BOB_CALLED state; if bob's call is up, then you know that Bob has made his call. Line 162: if (self.state == CAROL_CALLED and self.bridge2_bridged and I don't think you need the CAROL_CALLED state; if carol's call is up, then you know that Carol has made her call. -- To view, visit https://gerrit.asterisk.org/19 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1676801d90bcafc28ba25e8b6889f40ab08cc90e Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson mmichel...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-HasComments: Yes -- _ -- 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] 4542: DNS: Add NAPTR support and tests
On March 31, 2015, 10:25 a.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 447-449
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line447
Suggestion: since this is repeated after each check, you may want to
macro-tize it:
#define CHECK_END_OF_RECORD do { \
if (ptr = end_of_record) { \
return NULL; \
} } while (0)
Then you can just put:
ptr += 2;
CHECK_END_OF_RECORD;
Or something like that.
rmudgett wrote:
Doing this hides return points. Which adds a potential for memory and
ref leaks.
True, right now all of these do not perform cleanup. In fact, the entire
exercise here is mostly to figure out how long everything is so that things can
be allocated.
This is a long routine; sometimes it's nice to take repetitive code and squish
it down. If Mark feels like that injects too much risk, I have no problem with
the finding being dropped.
- Matt
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
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
[asterisk-dev] Change in testsuite[master]: Add SIP attended transfer for Asterisk 11.
Ashley Sanders has posted comments on this change. Change subject: Add SIP attended transfer for Asterisk 11. .. Patch Set 1: Code-Review-1 (1 comment) I think that most of this could be collapsed into the logic for the test for 12+. The AMI/bridging logic seems to be the only difference between the two versions. I think applying the strategy pattern will solve the problem of needing two tests for two disparate AMI APIs. This will help future-proof your code in that if any maintenance is required in the future, it will be a lot easier to apply that logic to one place rather than to n places :) https://gerrit.asterisk.org/#/c/20/1/tests/channels/SIP/sip_attended_transfer_11/attended_transfer.py File tests/channels/SIP/sip_attended_transfer_11/attended_transfer.py: After comparing this version against the version for the 12+ branch, the only significant difference is with respect to the ami bridging logic. I think you could just pull that piece out into it's own module and inject one strategy or the other based on the currently running asterisk version. This approach would greatly reduce the amount of code needed to test this feature. -- To view, visit https://gerrit.asterisk.org/20 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I48c7b6a9298552aa756d0c2f26afbd6a96d553b5 Gerrit-PatchSet: 1 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson mmichel...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-HasComments: Yes -- _ -- 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] Change in testsuite[master]: stasis: set a channel variable on websocket disconnect error
Mark Michelson has posted comments on this change.
Change subject: stasis: set a channel variable on websocket disconnect error
..
Patch Set 1:
(10 comments)
https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/ari_client.py
File tests/rest_api/applications/stasisstatus/ari_client.py:
Line 148: def on_channelcreated(self, message):
Unless I'm missing something, this, plus some of the other on_* functions
below, don't appear to be called during these tests. Can they be removed?
https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/observable_object.py
File tests/rest_api/applications/stasisstatus/observable_object.py:
Line 84: del self.__registrar[event][:]
I'm not 100% sure on this, but since you are using [:], that will return a copy
of the list at self.__registrar[event] rather than a reference to the actual
list, meaning that you aren't actually deleting the list at
self.__registrar[event]
I think that
del self.__registrar[event]
is actually what's wanted here.
Line 108: error += 'for event [{0}]; [Observers] is
None.'.format(event)
Nitpick: Start this string with a space, otherwise your error message will have
Could not register observersfor event...
Line 122: if self.__registrar[event] is None:
: msg += 'Instantiating the observers for event
{0}.'.format(event)
: LOGGER.debug(msg)
: self.__registrar[event] = list()
I may be misinterpreting your intent here, but I don't think this is going to
work how you expect it to. I've interpreted this block of code to mean that if
self.__registrar does not currently have the event key within it, then this
corrects the problem by setting self.__registrars[event] to be an empty list.
The problem with this is that if the key does not exist in self.__registrars,
then the if statement will throw a KeyError, not return None. You would need to
go with:
if event not in self.__registrar:
instead. Now, if you actually are trying to see if the value stored at the
event key in self.__registrar is None, then feel free to ignore this finding
completely.
Line 132: if self.__suspended 0:
: self.__suspended = 0
I don't imagine you actually want self.__suspended going less than 0, so this
should probably be = instead of
Line 143: def __validate(self, **kwargs):
This method feels a bit over-defensive. It's only ever called from two places,
and it has a single string passed to it. The method could be reduced down to
def __validate(self, event):
return True if event in self.__registrar else False
https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/run-test
File tests/rest_api/applications/stasisstatus/run-test:
Line 20: from stasisstatus.test_scenario_factory import build_scenarios
Hm, I don't see test_scenario_factory in this diff. Is it missing from the diff?
https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/test_case.py
File tests/rest_api/applications/stasisstatus/test_case.py:
Line 64: TestCase.ami_connect(self, ami)
Calling TestCase.ami_connect() isn't necessary since ami_connect is a virtual
method in the first place. It's designed for you to just do what you need to do
for your derived class and be on your way.
Line 126: TestCase.run(self)
:
: self.create_ami_factory()
I think that TestCase.run(self) will already create the AMI factory, so you
don't need to do it yourself here.
In fact, I don't think it's really necessary to override this method since It's
just doing the same thing as the parent (plus printing a debug message)
Line 142: TestCase.stop_reactor(self)
This notation is a bit odd. Why not just self.stop_reactor()?
--
To view, visit https://gerrit.asterisk.org/18
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7dadfd429bd30e9f07a531f47884d8c923fc13
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders asand...@digium.com
Gerrit-Reviewer: Mark Michelson mmichel...@digium.com
Gerrit-HasComments: Yes
--
_
-- 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] 4552: clang compiler warning: -Wsometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4552/#review14989 --- Ship it! Ship It! - Mark Michelson On March 28, 2015, 5:08 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4552/ --- (Updated March 28, 2015, 5:08 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wsometimes-uninitialized Diffs - /branches/13/funcs/func_math.c 433444 Diff: https://reviewboard.asterisk.org/r/4552/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] 4549: ARI: Add the ability to intercept hold and raise an event
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/#review14988 --- One thing to take into consideration here is that there are some places within Asterisk where we will send an AST_CONTROL_UNHOLD frame on a channel, even though it may not currently be on hold. This means you may send some unhold ARI events that don't match up with a previous hold event. This is probably worth documenting somewhere so that ARI application writers know what they might have to deal with. - Mark Michelson On March 28, 2015, 3:19 a.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/ --- (Updated March 28, 2015, 3:19 a.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24922 https://issues.asterisk.org/jira/browse/ASTERISK-24922 Repository: Asterisk Description --- For some applications - such as SLA - a phone pressing hold should not behave in the fashion that the Asterisk core would like it to. Instead, the hold action has some application specific behaviour associated with it - such as disconnecting the channel that initiated the hold; only playing MoH to channels in the bridge if the channels are of a particular type, etc. One way of accomplishing this is to use a framehook to intercept the hold/unhold frames, raise an event, and eat the frame. Tasty. The patch attached to this issue accomplished that as a new dialplan function, HOLD_INTERCEPT. In addition: * ARI now queues hold/unhold frames instead of indicating frames directly. This allows for the Stasis hold/unhold messages to be raised. * Some general cleanup of raising hold/unhold Stasis messages was done, including removing some RAII_VAR usage. Diffs - /branches/13/rest-api/api-docs/events.json 433677 /branches/13/res/stasis/control.c 433677 /branches/13/res/stasis/app.c 433677 /branches/13/res/ari/ari_model_validators.c 433677 /branches/13/res/ari/ari_model_validators.h 433677 /branches/13/main/stasis_channels.c 433677 /branches/13/main/manager_channels.c 433677 /branches/13/main/channel.c 433677 /branches/13/main/bridge_channel.c 433677 /branches/13/funcs/func_holdintercept.c PRE-CREATION /branches/13/CHANGES 433677 Diff: https://reviewboard.asterisk.org/r/4549/diff/ Testing --- See Gerrit reviews: https://gerrit.asterisk.org/#/c/16 https://gerrit.asterisk.org/#/c/17 Thanks, Matt Jordan -- _ -- 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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Ok you know that to prevent an unintendet assignment where a comparisson was
indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison between
double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean. Was
an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall -Werror)
to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an assignment
requires double parantheses.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
Because of the () where removed previously, they are now needed here.
main/stasis_message_router.c:82
===
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem).message_type != (value)) which would keep everything encapsulated,
but does result in a double negative, which is ugly as well.
main/stdtime/localtime.c:197/208
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
more of the same for
include/asterisk/dlinkedlists.h:422/431/469
main/astobj2_hash.c:768
-
Maybe one of you has a better/nicer solution.
Diffs
-
/branches/13/main/stdtime/localtime.c 433444
/branches/13/main/stasis_message_router.c 433444
/branches/13/main/format_cap.c 433444
/branches/13/main/astobj2_hash.c 433444
/branches/13/include/asterisk/vector.h 433444
/branches/13/include/asterisk/linkedlists.h 433444
/branches/13/include/asterisk/dlinkedlists.h 433444
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 7:15 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
clang compiler warning:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs (updated)
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- 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] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/#review14987 --- I don't understand the purpose of this warning. I tried searching for details about this warning flag on the internet and came up empty, so I can't find any documentation that explains what type of error this check is supposed to help avoid. It appears that the warning is intended to get rid of extra parentheses where they are unnecessary. The problem is that I don't see anything wrong with having them there, especially in macro definitions. - Mark Michelson On March 29, 2015, 5:14 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated March 29, 2015, 5:14 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. more of the same for include/asterisk/dlinkedlists.h:422/431/469 main/astobj2_hash.c:768 - Maybe one of you has a better/nicer solution. Diffs - /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- 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] Change in testsuite[master]: Rewrite sip_attended_transfer test to stop failing.
Mark Michelson has posted comments on this change. Change subject: Rewrite sip_attended_transfer test to stop failing. .. Patch Set 2: (6 comments) Thanks for the review! https://gerrit.asterisk.org/#/c/19/2/tests/channels/SIP/sip_attended_transfer/attended_transfer.py File tests/channels/SIP/sip_attended_transfer/attended_transfer.py: Line 36: class BobCallCallback(pj.CallCallback): Just as something to mention, BobCallCallback and CarolCallCallback can be Sounds good to me. Line 86: self.bridge1 = None Another fyi - The bridge1 and bridge2 could be made into a class and you co I like it. Line 121: self.call_carol() I think that you are calling this twice, once in the BobCallCallback.on_sta Well spotted! But, this is actually done intentionally. The reason is that we cannot guarantee the order of events. Bob's state may change to CONFIRMED before there are two channels in the Asterisk bridge, or it may happen the other way around. With this setup, they both attempt to call into the call_carol() function, and the call_carol() function will simply return early if the state is not such that calling Carol makes sense. Line 132: self.transfer_call() Here, too, this seems to be called twice; once in the CarolCallCallback.on_ And here it's the same deal as with your previous observation. Line 154: if (self.state == BOB_CALLED and self.bridge1_bridged and I don't think you need the BOB_CALLED state; if bob's call is up, then you This is a safeguard to ensure that we don't attempt to call Carol in a later stage of the test, say, after we've already performed the transfer. Looking again, I bet I could remove the check of the state from this function; however, the state itself as a class member is still necessary. The flow for a transfer goes as follows: Alice calls Bob, and they enter bridge 1. Alice calls Carol, and they enter bridge 2. Alice performs the transfer. Alice leaves both bridge 1 and 2. Now, the transfer code may move Bob out of bridge 1 and into bridge 2, or it may move Carol out of bridge 2 and into bridge 1. In either case, we detect the bridged state the same way as the original bridges with Alice: a BridgeEnter event with 2 channels in it. By maintaining the state of the test, we can determine whether a BridgeEnter with 2 channels means to continue on to the next state, or whether it means to hang up the calls because the test is complete. We also can detect if we get unexpected events and fail the test, as well. Line 162: if (self.state == CAROL_CALLED and self.bridge2_bridged and I don't think you need the CAROL_CALLED state; if carol's call is up, then See my reply about the BOB_CALLED state. -- To view, visit https://gerrit.asterisk.org/19 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1676801d90bcafc28ba25e8b6889f40ab08cc90e Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson mmichel...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Mark Michelson mmichel...@digium.com Gerrit-HasComments: Yes -- _ -- 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] 4528: dns: Add SRV recording parsing, sorting/weighting, and unit tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/#review14971 --- /trunk/main/dns_srv.c https://reviewboard.asterisk.org/r/4528/#comment25587 Let's talk alignment. In DNS, there is no padding used for variable-length elements. There is no guarantee that RRs (or their RDATA) lie on word-boundaries. This means that the data pointer passed to ast_dns_srv_alloc() may be pointing to data that is not word-aligned (or not even on a half-word). So in this case, you've overlaid a byte array on top of a struct that contains 16-bit unsigned integers. Typically, 16-bit values should be aligned on word or half-word boundaries, but because of the data layout of the DNS record, this cannot be guaranteed. This means you may be performing some unaligned reads here. Now the question is, how bad is this? I'm pretty sure that on x86 architectures, an unaligned read isn't a big deal, performance-wise, but I can't make any claims for other architectures simply because I don't know. Can someone more familiar with these sorts of things chime in with some info? /trunk/main/dns_srv.c https://reviewboard.asterisk.org/r/4528/#comment25593 Reading the language of RFC 2782, I'm unclear if what you're doing here is actually correct: To select a target to be contacted next, arrange all SRV RRs (that have not been ordered yet) in any order, except that all those with weight 0 are placed at the beginning of the list. I can interpret this two different ways: 1) Select all records of a given priority, and place them in a set, ensuring that the zero-weight records are at the beginning of the set. Then run the crazy weighting algorithm. 2) Select all records of a given priority. Take all 0-weighted records and place them into the sorted list of records first. Then run the crazy weighting algorithm on the rest of the records to determine their place in the list after the 0-weighted records. After re-reading a few times, I *think* they mean to do #1. If that's the case, then between the first highlighted list traversal and the while loop, you should place all 0-weight records at the front of temp_list. If they mean #2, then between the first highlighted list traversal and hte while loop, you should remove all 0-weighted records from temp_list and append them to the end of new_list. /trunk/tests/test_dns_srv.c https://reviewboard.asterisk.org/r/4528/#comment25594 A single red blob. - Mark Michelson On March 26, 2015, 6:50 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4528/ --- (Updated March 26, 2015, 6:50 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This change adds the following: 1. As SRV records are added to a result the information is parsed and stored away in additional storage in the record. The SRV API can then be used to return this information. 2. Before invoking the DNS query callback the list of records on the result are sorted based on priority and weight. 3. Unit tests have been added which verify the record parsing, sorting, and weighting. There are also some off nominal which cover the cases when an invalid/corrupt record is received. 4. A unit test has also been added to res_resolver_unbound which adds an SRV record to a zone and confirms it is retrieved and parsed correctly. Diffs - /trunk/tests/test_dns_srv.c PRE-CREATION /trunk/res/res_resolver_unbound.c 433370 /trunk/main/dns_srv.c 433370 /trunk/main/dns_core.c 433370 /trunk/include/asterisk/dns_internal.h 433370 Diff: https://reviewboard.asterisk.org/r/4528/diff/ Testing --- Executed unit tests and confirmed they pass. 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
[asterisk-dev] Change in testsuite[master]: Memory Debugging Improvements
Ashley Sanders has posted comments on this change. Change subject: Memory Debugging Improvements .. Patch Set 3: Would it be possible for you to post a sample output file? -- To view, visit https://gerrit.asterisk.org/15 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21634673508a01eea1f489c751d3cf75ea55cf06 Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Corey Farrell g...@cfware.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com Gerrit-HasComments: No -- _ -- 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] 4549: ARI: Add the ability to intercept hold and raise an event
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/#review14967 --- /branches/13/funcs/func_holdintercept.c https://reviewboard.asterisk.org/r/4549/#comment25583 Nit picky: is res really needed here? /branches/13/res/stasis/app.c https://reviewboard.asterisk.org/r/4549/#comment25584 I don't think these are needed. The sub_default_handler already does this and should be invoked for them. /branches/13/res/stasis/control.c https://reviewboard.asterisk.org/r/4549/#comment25585 This is substantially changing the behavior. ast_indicate will tell the channel to go on hold or off hold. ast_queue_hold/ast_queue_unhold will queue a frame as if the channel has said it is on hold or off hold. - Joshua Colp On March 28, 2015, 3:19 a.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4549/ --- (Updated March 28, 2015, 3:19 a.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24922 https://issues.asterisk.org/jira/browse/ASTERISK-24922 Repository: Asterisk Description --- For some applications - such as SLA - a phone pressing hold should not behave in the fashion that the Asterisk core would like it to. Instead, the hold action has some application specific behaviour associated with it - such as disconnecting the channel that initiated the hold; only playing MoH to channels in the bridge if the channels are of a particular type, etc. One way of accomplishing this is to use a framehook to intercept the hold/unhold frames, raise an event, and eat the frame. Tasty. The patch attached to this issue accomplished that as a new dialplan function, HOLD_INTERCEPT. In addition: * ARI now queues hold/unhold frames instead of indicating frames directly. This allows for the Stasis hold/unhold messages to be raised. * Some general cleanup of raising hold/unhold Stasis messages was done, including removing some RAII_VAR usage. Diffs - /branches/13/rest-api/api-docs/events.json 433677 /branches/13/res/stasis/control.c 433677 /branches/13/res/stasis/app.c 433677 /branches/13/res/ari/ari_model_validators.c 433677 /branches/13/res/ari/ari_model_validators.h 433677 /branches/13/main/stasis_channels.c 433677 /branches/13/main/manager_channels.c 433677 /branches/13/main/channel.c 433677 /branches/13/main/bridge_channel.c 433677 /branches/13/funcs/func_holdintercept.c PRE-CREATION /branches/13/CHANGES 433677 Diff: https://reviewboard.asterisk.org/r/4549/diff/ Testing --- See Gerrit reviews: https://gerrit.asterisk.org/#/c/16 https://gerrit.asterisk.org/#/c/17 Thanks, Matt Jordan -- _ -- 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] 4519: Asterisk: stasis: set a channel variable on websocket disconnect error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4519/#review14968 --- Ship it! ./branches/13/apps/app_stasis.c https://reviewboard.asterisk.org/r/4519/#comment25586 Nitpick: A failure occurred when executing the Stasis application. Otherwise good. - Joshua Colp On March 28, 2015, 5:38 a.m., Ashley Sanders wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4519/ --- (Updated March 28, 2015, 5:38 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24802 https://issues.asterisk.org/jira/browse/ASTERISK-24802 Repository: Asterisk Description --- When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis). To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS. The possible values for STASISSTATUS are: SUCCESS - The channel has exited Stasis without any failures FAILED - Something caused Stasis to croak. Some (not all) possible reasons for this: - The app registry is not instantiated; - The app requested is not registered; - The app requested is not active; - Stasis couldn't send a start message ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.) And, here it is! https://reviewboard.asterisk.org/r/4520 Diffs - ./branches/13/apps/app_stasis.c 433290 Diff: https://reviewboard.asterisk.org/r/4519/diff/ Testing --- The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/ The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable: 1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis correctly assigns SUCCESS to STASISSTATUS. 2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied. 3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied. Thanks, Ashley Sanders -- _ -- 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] Asterisk to Git Migration Update
On Tue, Mar 31, 2015 at 1:48 PM, Russell Bryant russ...@russellbryant.net wrote: On Tue, Mar 31, 2015 at 2:32 PM, Matthew Jordan mjor...@digium.com wrote: snip There's a large amount of intervening work that has to be done, and some questions that still need to be answered. In no particular order: (1) In test runs of building the Asterisk repo, I've successfully managed to pull in the team repos that are still 'active'. Since Gerrit acts as the canonical Git repo, we can set it up so that direct pushes to team branches are allowed and don't require a code review - although code reviews can be done if someone desires it. For those who use team branches a lot, does this sound acceptable? What's the benefit of trying to keep team branches at all vs. just letting people use their own git trees hosted on one of the many personal git hosting options (github or whatever) ? If it's about several people collaborating on a branch, that makes sense to me to do as a feature branch in gerrit, but it seems like the exception, not the rule. I think the primary benefit is having a place for collaborators to put stuff. If anything - since we won't have to worry about SSL access to SVN - the move to Gerrit will make that *must* easier for people. The secondary benefit is having a location for people who have signed a CLA to push code, and implicit in that push, that they've licensed that code back to the Asterisk project. snip (b) sounds the least painful overall to me. People consuming releases shouldn't notice a difference, right? Yeah, I think I'd rather just bite the bullet and get the repo set up right with as few weird things lingering around as possible. The only potential issue is requiring libxml2 (as we didn't pull in the bundled mxml) - but we may as well deal with the fallout now. (4) Asterisk records the SVN revision in each file using the special keyword $Revision:. This is then registered in a linked list for retrieval by the CLI/AMI. Unfortunately, Git doesn't support this concept, as adding data into a file after commit would change the checksum . It does allow doing this on checkout via the $Id$ keyword, which may be an acceptable workaround. That whole thing seems questionably useful, anyway. Just removing it is another option. True. I'm not sure how much value people get from knowing the SVN revision of a file... much less a checksum. -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] Change in testsuite[master]: Memory Debugging Improvements
Ashley Sanders has posted comments on this change. Change subject: Memory Debugging Improvements .. Patch Set 3: Code-Review-1 (1 comment) I only have one finding - a misspelling. https://gerrit.asterisk.org/#/c/15/3/contrib/valgrind/summary-lines.xsl File contrib/valgrind/summary-lines.xsl: Line 28:cols col1=Definately Lost Definately should be spelled, Definitely (I assume this is a fat-finger since it is spelled correctly in the 'kind' reference above :p) -- To view, visit https://gerrit.asterisk.org/15 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21634673508a01eea1f489c751d3cf75ea55cf06 Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Corey Farrell g...@cfware.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com Gerrit-HasComments: Yes -- _ -- 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] 4519: Asterisk: stasis: set a channel variable on websocket disconnect error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4519/ --- (Updated March 31, 2015, 4:54 p.m.) Review request for Asterisk Developers. Changes --- Fixed the docstring according to Joshua Colp's suggestion. Bugs: ASTERISK-24802 https://issues.asterisk.org/jira/browse/ASTERISK-24802 Repository: Asterisk Description --- When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis). To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS. The possible values for STASISSTATUS are: SUCCESS - The channel has exited Stasis without any failures FAILED - Something caused Stasis to croak. Some (not all) possible reasons for this: - The app registry is not instantiated; - The app requested is not registered; - The app requested is not active; - Stasis couldn't send a start message ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.) And, here it is! https://reviewboard.asterisk.org/r/4520 Diffs (updated) - ./branches/13/apps/app_stasis.c 433794 Diff: https://reviewboard.asterisk.org/r/4519/diff/ Testing --- The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/ The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable: 1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis correctly assigns SUCCESS to STASISSTATUS. 2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied. 3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied. Thanks, Ashley Sanders -- _ -- 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] 4108: Weak Proxy Objects
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4108/#review14995
---
This approach is much simpler and easier to understand than previous attempts.
Since it is more general it could be used to avoid mutual ref issues such as
between the stasis_topic and stasis_subscription as described in main/stasis.c.
All of the issues I've pointed out are minor.
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25613
You could do it this way:
/*! \brief Macro which must be used at the beginning of each sorcery
capable object */
#define SORCERY_OBJECT(details)\
struct { \
struct ast_sorcery_object_details details; \
} \
This way you aren't using a macro that alters the declaration syntax.
Need to document that principle intent of user defined fields on weak proxy
object structs is to serve as immutable container keys for the real object.
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25622
Why not use the OBJ_NOLOCK flag?
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25623
Why not use the OBJ_NOLOCK flag?
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25624
Why not use the OBJ_NOLOCK flag?
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25626
...as many times.
/trunk/include/asterisk/astobj2.h
https://reviewboard.asterisk.org/r/4108/#comment25625
Why not use the OBJ_NOLOCK flag?
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25615
For alignment purposes, this should be grouped with the other pointer
(destructor_fn).
Also ao2_weak - ao2_weakproxy
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25616
How about renaming to:
IS_AO2_MAGIC_BAD()
if (IS_AO2_MAGIC_BAD()) {
ast_log(LOG_ERROR, Bad magic number\n)
}
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25620
Bumping weakproxy is not necessary here. It is just done as a convienience
to later coding.
There is an expense to ast_atomic_fetchadd_int() because of syncing
processors.
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25621
Isn't the shell game you are playing with the real object's ref count going
to mess up the DEBUG_REF output?
How about this code:
Code assumes that weakproxy ref wasn't bumped when set above.
if weakproxy
if current_value == 1
unlink real and weak
unlock weakproxy
if current_value == 1
run weakproxy callbacks (locking between each callback or locking to
run all)
unref weakproxy
unref real
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25617
I think you need to have a weakproxy destructor callback that performs a
sanity check on the proxy object to ensure that it is not still linked with a
real object.
Or even better, make the ao2_ref code do this check when it is about to
destroy a weakproxy object. This check can also destroy anything in the notify
list for the case if the real object was not set but subscribers were.
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25618
Format nit, break lines at weaker binary operations and put the operation
at the beginning of the indented new line:
if (!weakproxy_internal
|| weakproxy_internal-priv_data.magic != AO2_WEAK) {
return -1;
}
if (!obj_internal
|| obj_internal-priv_data.weakptr
|| obj_internal-priv_data.magic != AO2_MAGIC) {
return -1;
}
This way you aren't as likely to miss the binary operator because it isn't
hiding at the end of a possibly still long line.
And don't use spaces for indentation.
/trunk/main/astobj2.c
https://reviewboard.asterisk.org/r/4108/#comment25619
I don't see that this goto really helps here.
Invert the test and pull the skipped code into the then block.
/trunk/tests/test_astobj2_weaken.c
https://reviewboard.asterisk.org/r/4108/#comment25627
Digium didn't create this file.
/trunk/tests/test_astobj2_weaken.c
https://reviewboard.asterisk.org/r/4108/#comment25633
Value not checked at end of test to ensure that it got destroyed and there
were no leaks.
/trunk/tests/test_astobj2_weaken.c
https://reviewboard.asterisk.org/r/4108/#comment25630
++*i
avoids the need for parentheses and post increment isn't really needed.
/trunk/tests/test_astobj2_weaken.c
Re: [asterisk-dev] [Code Review] 4519: Asterisk: stasis: set a channel variable on websocket disconnect error
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4519/ --- (Updated March 31, 2015, 5 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433839 Bugs: ASTERISK-24802 https://issues.asterisk.org/jira/browse/ASTERISK-24802 Repository: Asterisk Description --- When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis). To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS. The possible values for STASISSTATUS are: SUCCESS - The channel has exited Stasis without any failures FAILED - Something caused Stasis to croak. Some (not all) possible reasons for this: - The app registry is not instantiated; - The app requested is not registered; - The app requested is not active; - Stasis couldn't send a start message ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.) And, here it is! https://reviewboard.asterisk.org/r/4520 Diffs - ./branches/13/apps/app_stasis.c 433794 Diff: https://reviewboard.asterisk.org/r/4519/diff/ Testing --- The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/ The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable: 1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis correctly assigns SUCCESS to STASISSTATUS. 2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied. 3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied. Thanks, Ashley Sanders -- _ -- 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] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14999 --- I dislike this warning. It is more nuisance than helpful. I once worked with compiler that had a similar warning. The lengths you sometimes have to go to eradicate the the warning can be extream. There are several places in this patch that change code behavior and introduce bugs. - rmudgett On March 28, 2015, 7:44 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 7:44 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- 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] 4555: clang compiler warning: fixes for tests to be compiled using clang
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/#review15000
---
/branches/13/main/utils.c
https://reviewboard.asterisk.org/r/4555/#comment25642
Revert this. This change could affect the callers of the function since
you are changing the API. However, no callers currently care about the return
value.
You changed this because clang had a problem in test_semi1() in
tests/test_strings.c. There is a better way.
/branches/13/tests/test_acl.c
https://reviewboard.asterisk.org/r/4555/#comment25635
prototype unnecessary
/branches/13/tests/test_acl.c
https://reviewboard.asterisk.org/r/4555/#comment25636
Guidelines: curly on its own line for start of functions
res must be changed to a pointer so the value set in res can be passed out.
/branches/13/tests/test_linkedlists.c
https://reviewboard.asterisk.org/r/4555/#comment25637
Revert the changes in this file. They are for the -Wparentheses-equality
option.
/branches/13/tests/test_stringfields.c
https://reviewboard.asterisk.org/r/4555/#comment25638
Use %d instead of %i. %d is more portable since it has been around since
KR.
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25641
Revert this.
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25646
Is clang returning a NULL pointer when passed a zero length? If so this
could be a problem througout the codebase since the code assumes that the
function can never fail.
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25640
Did you mean ast_alloca() or ast_strdupa()?
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25639
Guidelines: No C++ comments.
Delete this comment line.
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25643
} else if (test_len == 0 {
test2 = ;
}
/branches/13/tests/test_strings.c
https://reviewboard.asterisk.org/r/4555/#comment25644
Revert now that the clang ast_alloca() problem is worked around.
- rmudgett
On March 29, 2015, 5:49 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/
---
(Updated March 29, 2015, 5:49 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917
Repository: Asterisk
Description
---
clang's static analyzer will throw quite a number warnings / errors during
compilation, some of which can be very helpfull in finding corner-case bugs.
fixes for tests to be compiled using clang
Diffs
-
/branches/13/tests/test_strings.c 433444
/branches/13/tests/test_stringfields.c 433444
/branches/13/tests/test_sched.c 433444
/branches/13/tests/test_linkedlists.c 433444
/branches/13/tests/test_acl.c 433444
/branches/13/main/utils.c 433444
Diff: https://reviewboard.asterisk.org/r/4555/diff/
Testing
---
executing the tests one-by-one works fine (completes to end) (skipping
/main/stdtime) -
test show results failed:
=== /main/message/ ==
FAIL test_message_queue_handler_nom /main/message/ 31036ms
[test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test
timed out while waiting for handler to get message
Not sure if this is actually a fail or just a timeout. WIP
=== /main/strings/ ==
FAIL escape_semicolons /main/strings/ 1ms
[Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const
char *, char *, int): FRACK!, Failed assertion string != NULL outbuf !=
NULL (0)
- explainable by the change made to the source. ast_alloca(0) is not being
executed - test2 = NULL: need to resolv the open question how to handle
ast_alloca(0) before making any further changes.
(With revision 5 of this code, this test now passes without a problem, had to
fix both the test and the function being tested though)
=== /main/stdtime ==
test execute all fails, caused by the /main/stdtime/ test.
START /main/stdtime/ - timezone_watch
[test_time.c:enum ast_test_result_state test_timezone_watch(struct
ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing
deletion test...
j62747*CLI
CLI becomes unresponsive / no further command completion for example.
Guess this will need a little further investigation. Maybe the source changes
made to main/stdtime/ where not completely correct.
Seems to be caused by
Re: [asterisk-dev] [Code Review] 4108: Weak Proxy Objects
On March 31, 2015, 5:52 p.m., rmudgett wrote: /trunk/main/astobj2.c, line 798 https://reviewboard.asterisk.org/r/4108/diff/5/?file=71780#file71780line798 I think you need to have a weakproxy destructor callback that performs a sanity check on the proxy object to ensure that it is not still linked with a real object. Or even better, make the ao2_ref code do this check when it is about to destroy a weakproxy object. This check can also destroy anything in the notify list for the case if the real object was not set but subscribers were. Once linked to a real object the weakproxy cannot be destroyed until after no references to the real object exist. Remember that until the real object is destroyed it holds a reference to the weakproxy. As for subscriptions I will have to look into this more, but my intention for subscriptions is a way to request notification as soon as the weakproxy points to NULL. So in the case of adding a subscription when no real object is linked, I think we should run the callback immediately and not add it to the subscription list. On March 31, 2015, 5:52 p.m., rmudgett wrote: /trunk/main/astobj2.c, lines 450-451 https://reviewboard.asterisk.org/r/4108/diff/5/?file=71780#file71780line450 Isn't the shell game you are playing with the real object's ref count going to mess up the DEBUG_REF output? How about this code: Code assumes that weakproxy ref wasn't bumped when set above. if weakproxy if current_value == 1 unlink real and weak unlock weakproxy if current_value == 1 run weakproxy callbacks (locking between each callback or locking to run all) unref weakproxy unref real Lets use realobj to represent the variable in the callers scope (non-ao2 code), user_data is the variable in internal_ao2_ref. This shell game results in REF_DEBUG logs showing the 2nd to last reference being released by ao2_ref(user_data, -1) a couple lines below your finding, just prior to the user releasing the last reference. REF_DEBUG is produced after internal_ao2_ref completes, so it's impossible for the unref's in this block to be logged after ao2_ref(realobj, -1). So without this REF_DEBUG would say that the object was destroyed due to ao2_ref(user_data, -1) before the 2nd to last reference was released. This REF_DEBUG output was verified using the provided test. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/#review14995 --- On March 4, 2015, 4:43 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/ --- (Updated March 4, 2015, 4:43 p.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Repository: Asterisk Description --- This implements weak references. The weakproxy object is a real ao2 with normal reference counting of its own. When a weakproxy is pointed to a normal object they hold references to each other. The normal object is automatically freed when a single reference remains (the weakproxy). The weakproxy also supports subscriptions that will notify callbacks when the normal object is about to be destroyed. Diffs - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 432445 /trunk/include/asterisk/astobj2.h 432445 Diff: https://reviewboard.asterisk.org/r/4108/diff/ Testing --- Ran the included test with REF_DEBUG enabled under valgrind. No reference leaks or improper memory access. Though this does not test for races, I don't know of an automated way to do that. 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
