[asterisk-dev] Change in testsuite[master]: Enable support for directory containing custom tests.
Corey Farrell has uploaded a new change for review. https://gerrit.asterisk.org/27 Change subject: Enable support for directory containing custom tests. .. Enable support for directory containing custom tests. This change enables use of 'tests/custom/' as the place for custom tests. If tests/custom/tests.yaml exists, it will be loaded. This allows custom tests to be added without the need to modify any tests.yaml files under Asterisk Testsuite source control. This recommended use of this feature is to clone another repository into tests/custom, adding tests specific to your own business, clients, or products using Asterisk. Change-Id: Iff45fe5574900b5c5c77e3132984659133baadfd --- M .gitignore M README.txt M runtests.py M tests/tests.yaml 4 files changed, 19 insertions(+), 1 deletion(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/27/27/1 diff --git a/.gitignore b/.gitignore index b7f92b6..2a84c60 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ /astroot /contrib/valgrind/suppressions.txt /logs +/tests/custom diff --git a/README.txt b/README.txt index f6c5b80..4ebbd94 100644 --- a/README.txt +++ b/README.txt @@ -21,6 +21,7 @@ 5) Test Configuration 6) Tests in Python 7) Tests in Lua +8) Custom Tests @@ -543,5 +544,19 @@ + +--- 8) Custom Tests + + +The testsuite supports automatic use of custom tests. This feature is +activated by creating tests/custom/tests.yaml to list your tests and/or folders +of tests. Any files created in tests/custom will be ignored by the Asterisk +testsuite repository. This folder is designed to be used for tests that are +not appropriate for inclusion in the common testsuite. This can include tests +specific for your business, clients or Asterisk based product. + + + + diff --git a/runtests.py b/runtests.py index f745266..c4d2cda 100755 --- a/runtests.py +++ b/runtests.py @@ -335,7 +335,8 @@ try: f = open(%s/%s % (test_dir, TESTS_CONFIG), r) except IOError: -print Failed to open %s/%s % (test_dir, TESTS_CONFIG) +if test_dir != tests/custom: +print Failed to open %s/%s % (test_dir, TESTS_CONFIG) return tests except: print Unexpected error: %s % sys.exc_info()[0] diff --git a/tests/tests.yaml b/tests/tests.yaml index 59f66bb..52b63aa 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -1,6 +1,7 @@ # Enter tests here in the order they should be considered for execution: tests: - test: 'example' +- dir: 'custom' - test: 'dynamic-modules' - dir: 'manager' - dir: 'cdr' -- To view, visit https://gerrit.asterisk.org/27 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iff45fe5574900b5c5c77e3132984659133baadfd Gerrit-PatchSet: 1 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Corey Farrell g...@cfware.com -- _ -- 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] 4578: res_pjsip_phoneprov_provider: Fix leaked OBJ_MULTIPLE iterator.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4578/#review15041 --- Ship it! Ship It! - George Joseph On April 3, 2015, 7:31 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4578/ --- (Updated April 3, 2015, 7:31 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24935 https://issues.asterisk.org/jira/browse/ASTERISK-24935 Repository: Asterisk Description --- res_pjsip_phoneprov_provider was using ao2_callback with OBJ_MULTIPLE, then ignoring the return. This resulted in a reference leak. Added OBJ_NODATA flag. Diffs - /branches/13/res/res_pjsip_phoneprov_provider.c 433966 Diff: https://reviewboard.asterisk.org/r/4578/diff/ Testing --- Started and stopped Asterisk, REF_DEBUG now showing no leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/#review15039 --- /trunk/main/manager.c https://reviewboard.asterisk.org/r/4391/#comment25718 If we successfully ran the command, it seems unsafe to claim failure. We have to assume the the caller doesn't actually care about any of the CLI output, they only care about having the command perform an action. So I think we need to respond with success if the command ran. I'm leaning towards thinking that this error should be provided through a single Output: Command response construction error\r\n, so move astman_start_ack to just below ast_cli_command. On a related issue, there are a couple errors that can occur in ast_cli_command_full which print error messages and return success. I don't know if it's safe to modify ast_cli_command_full to return errors for more situations, it might be worth looking at to allow us to trust the return value of ast_cli_command_full. CLI commands themselves can return an error, but this error is not returned by ast_cli_command_full. It would be nice if this action could use the return value from ast_cli_command_full to determine if it should respond success or failure. - Corey Farrell On March 25, 2015, 12:34 a.m., gareth wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/ --- (Updated March 25, 2015, 12:34 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24730 https://issues.asterisk.org/jira/browse/ASTERISK-24730 Repository: Asterisk Description --- This patch adds a blank line between the headers and the output in the Command action response. The reason for this change is to make it easier to determine where the headers end and the output from the command starts. Currently, to parse a response to a Command action, one has to: 1. Read one line, if line is Response: Error, parse the remaining as a standard AMI response and stop. 2. Read one more line - or two if you used an ActionID - those lines are the headers. 3. Then read everything up to --END COMMAND--\r\n\r\n. That could be changed to: 1. Read standard AMI response. 2. If Response: Follows, then read everything up to --END COMMAND--\r\n\r\n. The AMI version has been increased to 2.8.0 as this is a protocol change and so that clients detect the new behavior. Adding a blank line should not cause older parsers to fail as they have to read everything up to --END COMMAND--\r\n\r\n anyway. Adding a blank line will also not cause the AMI to HTML/XML encoder to fail to separate the headers from the output as it checks for the presence of a : character, which a blank line does not contain. Diffs - /trunk/main/manager.c 433198 /trunk/include/asterisk/manager.h 433198 /trunk/UPGRADE.txt 433198 Diff: https://reviewboard.asterisk.org/r/4391/diff/ Testing --- Connected to manager, issued 'core show uptime' command and verified that there was a blank line between the headers and output. Thanks, gareth -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] Change in testsuite[master]: Add a test for PJSIP t38 with authentication based on normal...
Hello Ashley Sanders, I'd like you to reexamine a change. Please visit https://gerrit.asterisk.org/22 to look at the new patch set (#4). Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test .. Add a test for PJSIP t38 with authentication based on normal t38 test Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 --- A tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast1/pjsip.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/pjsip.conf A tests/fax/pjsip/t38_with_auth/run-test A tests/fax/pjsip/t38_with_auth/send.tiff A tests/fax/pjsip/t38_with_auth/test-config.yaml M tests/fax/pjsip/tests.yaml 8 files changed, 175 insertions(+), 0 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/22/22/4 -- To view, visit https://gerrit.asterisk.org/22 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 Gerrit-PatchSet: 4 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com -- _ -- 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]: Address review feedback
Jonathan Rose has uploaded a new change for review.
https://gerrit.asterisk.org/28
Change subject: Address review feedback
..
Address review feedback
Change-Id: If37cf20857ae3c0b35e0637a0a2cb7e7d6226df6
---
M tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
M tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
M tests/fax/pjsip/t38_with_auth/run-test
M tests/fax/pjsip/t38_with_auth/test-config.yaml
4 files changed, 62 insertions(+), 52 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/28/28/1
diff --git a/tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
b/tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
index 55a81b4..83e0d5b 100644
--- a/tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
+++ b/tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
@@ -1,7 +1,7 @@
[sendfax]
exten = 1234,1,noop
-exten = 1234,n,SendFax(${ASTDATADIR}/send.tiff)
+ same = n,SendFax(${ASTDATADIR}/send.tiff)
exten = h,1,noop
-exten = h,n,UserEvent(FaxStatus,operation: send,status:
${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
+ same = n,UserEvent(FaxStatus,operation: send,status:
${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
diff --git a/tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
b/tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
index 03f2714..8d5dc7f 100644
--- a/tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
+++ b/tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
@@ -1,6 +1,6 @@
[receivefax]
exten = 1234,1,noop
-exten = 1234,n,ReceiveFax(${ASTDATADIR}/receive.tiff)
+ same = n,ReceiveFax(${ASTDATADIR}/receive.tiff)
exten = h,1,noop
-exten = h,n,UserEvent(FaxStatus,operation: receive,status:
${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
+ same = n,UserEvent(FaxStatus,operation: receive,status:
${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
diff --git a/tests/fax/pjsip/t38_with_auth/run-test
b/tests/fax/pjsip/t38_with_auth/run-test
old mode 100644
new mode 100755
index 8af0d4d..30fc635
--- a/tests/fax/pjsip/t38_with_auth/run-test
+++ b/tests/fax/pjsip/t38_with_auth/run-test
@@ -1,8 +1,9 @@
#!/usr/bin/env python
# vim: sw=3 et:
'''
-Copyright (C) 2011, Digium, Inc.
+Copyright (C) 2015, Digium, Inc.
Matthew Nicholson mnichol...@digium.com
+Jonathan Rose jr...@digium.com
This program is free software, distributed under the terms of
the GNU General Public License Version 2.
@@ -11,73 +12,81 @@
import sys
import logging
import os
-import re
import shutil
-from twisted.internet import reactor
-
sys.path.append(lib/python)
-from asterisk.asterisk import Asterisk
+
+from twisted.internet import reactor
from asterisk.test_case import TestCase
-logger = logging.getLogger(__name__)
+LOGGER = logging.getLogger(__name__)
+
class T38Test(TestCase):
- event_count = 0
- success_count = 0
+Test class for performing the test. Manages files, originates FAX call,
+and monitors a user event to verify that the fax completed successfully on
+the sender end of the call.
+
- def __init__(self):
- TestCase.__init__(self)
- self.reactor_timeout = 120
- self.create_asterisk(2)
+def __init__(self):
+Create Asterisk instances and copy our test fax to where the sender
+will send it from.
+
- # copy the tiff file we are going to send to a good known location
- shutil.copy(%s/send.tiff %
(os.path.dirname(os.path.realpath(__file__)),), %s%s % (self.ast[0].base,
self.ast[0].directories['astdatadir']))
+super(T38Test, self).__init__()
+self.reactor_timeout = 120
+self.create_asterisk(2)
- def ami_connect(self, ami):
- if ami.id == 0:
+# copy the tiff file we are going to send to a good known location
+shutil.copy(%s/send.tiff % (os.path.dirname(
+os.path.realpath(__file__)),), %s%s %
+(self.ast[0].base, self.ast[0].directories['astdatadir']))
- ami.registerEvent('UserEvent', self.fax_result)
- df = ami.originate(PJSIP/ast2-t38/sip:1234@127.0.0.2, sendfax,
1234, 1)
+def ami_connect(self, ami):
+Upon connecting to AMI on the first Asterisk instance, originate
+a call that will send the fax to the second Asterisk instance. Also
+register for UserEvents.
+
- def handle_failure(reason):
-logging.error(error sending originate:)
-logging.error(reason.getTraceback())
-self.stop_reactor()
+if ami.id == 0:
+ami.registerEvent('UserEvent', self.fax_result)
+deferred = ami.originate(PJSIP/ast2-t38/sip:1234@127.0.0.2,
+ sendfax, 1234, 1)
+
[asterisk-dev] Change in testsuite[master]: non_stasis_bridge_to_stasis_bridge: Update regex for ami events
John Bigelow has posted comments on this change. Change subject: non_stasis_bridge_to_stasis_bridge: Update regex for ami events .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.asterisk.org/25 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0a3bcb1a0df7e7bdca02be827288f5f08b5140ce Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Kevin Harwell kharw...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-Reviewer: John Bigelow jbige...@digium.com Gerrit-Reviewer: Kevin Harwell kharw...@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
[asterisk-dev] [Code Review] 4579: res_pjsip_messaging: Serialize outgoing MESSAGEs
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4579/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24937 https://issues.asterisk.org/jira/browse/ASTERISK-24937 Repository: Asterisk Description --- Because res_pjsip_messaging throws all messages to send into the threadpool, there is no guarantee that consecutive outgoing messages from the same dialplan execution will be sent in the same order. So for instance, if you had the following dialplan: exten = hello,1,NoOp() same = n,SendMessage(hello) same = n,SendMessage(world) It would be expected that the hello message would be sent before the world message. However, it cannot be guaranteed this will happen with the current threadpool usage. The patch on this review introduces a serializer for outgoing MESSAGE requests from Asterisk. This ensures that all MESSAGE requests are sent in the same order that they are processed in the dialplan. Diffs - /branches/13/res/res_pjsip_messaging.c 433838 Diff: https://reviewboard.asterisk.org/r/4579/diff/ Testing --- The bug itself is incredibly difficult to have happen under normal circumstances, but I have confirmed that this patch has not hindered operations any. 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 a test for PJSIP t38 with authentication based on normal...
Jonathan Rose has uploaded a new patch set (#2). Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive .. Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive the call and direct it to an extension that runs receiveFax. As the fax completes, user events are issued to verify that the FAX completed successfully. ASTERISK-24933 Change-Id: If37cf20857ae3c0b35e0637a0a2cb7e7d6226df6 --- M tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf M tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf M tests/fax/pjsip/t38_with_auth/run-test M tests/fax/pjsip/t38_with_auth/test-config.yaml 4 files changed, 62 insertions(+), 52 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/28/28/2 -- To view, visit https://gerrit.asterisk.org/28 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If37cf20857ae3c0b35e0637a0a2cb7e7d6226df6 Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com -- _ -- 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/ --- (Updated April 3, 2015, 12:58 p.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Changes --- * Address Richards findings. * Enable REF_DEBUG. * Rename some variables in the test to make it easier to trace, added ao2 REF_DEBUG tag's. * Add JIRA ticket. Bugs: ASTERISK-24936 https://issues.asterisk.org/jira/browse/ASTERISK-24936 Repository: Asterisk Description (updated) --- 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 it does not point to any real object. Diffs (updated) - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 433964 /trunk/include/asterisk/astobj2.h 433964 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
Re: [asterisk-dev] [Code Review] 4577: res_pjsip_t38: T38 fax fails when using authentication with PJSIP sender
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/#review15046 --- /certified/branches/13.1/res/res_pjsip_t38.c https://reviewboard.asterisk.org/r/4577/#comment25722 s/for the channel/for the endpoint/ - Jonathan Rose On April 3, 2015, 12:03 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/ --- (Updated April 3, 2015, 12:03 p.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24933 https://issues.asterisk.org/jira/browse/ASTERISK-24933 Repository: Asterisk Description --- Description: Two Asterisk boxes each have the other as endpoints with authentication set. First Asterisk box originates a call to the second using the PJSIP endpoint. The first Asterisk box uses an extension with sendfax, the second uses an extension with receivefax. The session starts fairly normally, but resolution never appears in fax show session output. After a while (~25 seconds) the call drops and the fax fails. Error messages shown are as follows: Sender: The call dropped prematurely Receiver: Disconnected after permitted retries Note that when not using authentication, the FAX will complete as expected. When using chan_sip as the sender to a receiver of chan_pjsip, the FAX will also complete as expected with authentication, but if chan_pjsip is the sender it will fail regardless of whether the recipient is chan_sip or chan_pjsip. The problem is caused by duplication of a framehook in res_pjsip_t38 which occurs on the second invite sent out when responding to the auth challenge. Fix: In order to fix this, I added a simple flag to the pjsip session struct that would be raised when the framehook is first attached to prevent duplication. I wouldn't be surprised if there were a better way to do this. Diffs - /certified/branches/13.1/res/res_pjsip_t38.c 433316 Diff: https://reviewboard.asterisk.org/r/4577/diff/ Testing --- I've duplicated and modified the t38 PJSIP fax test in the testsuite to include authentication. It fails without the patch and passes with the patch. I also tested this locally with my two Asterisk machines in the above scenario. I'll be linking the test after putting it on Gerrit. Thanks, Jonathan Rose -- _ -- 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/#review15047 --- When I try to view the review, Review Board complains that it cannot display the changes because the diff uploaded did not apply cleanly. Please ensure that your working copy of the files being altered is up to date. Also, if you're not already using it, there are some very useful command line tools you can use for uploading reviews to review board. See the Posting Code to Review Board section on this wiki page: https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage - Mark Michelson On April 3, 2015, 4:56 p.m., Rodrigo Ramirez Norambuena wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4571/ --- (Updated April 3, 2015, 4:56 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/samples/cel_pgsql.conf.sample 433966 /trunk/cel/cel_pgsql.c 433966 /trunk/CHANGES 433966 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
[asterisk-dev] Change in testsuite[master]: non_stasis_bridge_to_stasis_bridge: Update regex for ami events
Kevin Harwell has posted comments on this change. Change subject: non_stasis_bridge_to_stasis_bridge: Update regex for ami events .. Patch Set 3: Updated the per review based on feedback and other findings. A couple of things: 1. I am an idiot and my eyes were playing tricks on me, but there is actually nothing wrong with the regex parsing of '-'. The regex on those particular events were missing some text. 2. To answer John and Corey's question about why that particular event action was moved: There original reason was after applying the Asterisk fix the test was still failing due to what seemed like bob being hung up too early. I moved it thinking there might have been bit of a race between events, but it turned out the problem was due to the regex being wrong on some events. Then I forgot to move it back. I've now moved it back and will leave the comment there since it applies again. -- To view, visit https://gerrit.asterisk.org/25 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0a3bcb1a0df7e7bdca02be827288f5f08b5140ce Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Kevin Harwell kharw...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-Reviewer: John Bigelow jbige...@digium.com Gerrit-Reviewer: Kevin Harwell kharw...@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
[asterisk-dev] Change in testsuite[master]: Add a test for PJSIP t38 with authentication based on normal...
Jonathan Rose has abandoned this change. Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test .. Abandoned I'm abandoning this one since the patch got mangled. Go over to c/28 -- To view, visit https://gerrit.asterisk.org/22 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 Gerrit-PatchSet: 4 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4581: build: Fixes for gcc 5 compilation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4581/ --- Review request for Asterisk Developers and Scott Griepentrog. Bugs: ASTERISK-24932 https://issues.asterisk.org/jira/browse/ASTERISK-24932 Repository: Asterisk Description --- These are fixes for compilation under gcc 5.0... chan_sip.c:In parse_request needed to make 'lim' unsigned. inline_api.h: Needed to add a check for '__GNUC_STDC_INLINE__' to detect C99 inline semantics (same as clang). ccss.c:In ast_cc_set_parm, needed to fix weird comparison. dsp.c: Needed to work around a possible compiler bug. It was throwing an array-bounds error but neither sgriepentrog, rmudgett nor I could figure out why. manager.c: In action_atxfer, needed to correct an array allocation. If I can reproduce the possible gcc bug in a short test case, I'll submit it upstream. This patch will go to 11, 13, trunk. Reported-by: Jeffrey Ollie Diffs - branches/11/main/manager.c 433966 branches/11/main/dsp.c 433966 branches/11/main/ccss.c 433966 branches/11/include/asterisk/inline_api.h 433966 branches/11/channels/chan_sip.c 433966 Diff: https://reviewboard.asterisk.org/r/4581/diff/ Testing --- Ran unit and testsuite tests. No differences detected. Thanks, George Joseph -- _ -- 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] 4577: res_pjsip_t38: T38 fax fails when using authentication with PJSIP sender
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/ --- (Updated April 3, 2015, 12:03 p.m.) Review request for Asterisk Developers and Joshua Colp. Changes --- Convert flag to datastore Bugs: ASTERISK-24933 https://issues.asterisk.org/jira/browse/ASTERISK-24933 Repository: Asterisk Description --- Description: Two Asterisk boxes each have the other as endpoints with authentication set. First Asterisk box originates a call to the second using the PJSIP endpoint. The first Asterisk box uses an extension with sendfax, the second uses an extension with receivefax. The session starts fairly normally, but resolution never appears in fax show session output. After a while (~25 seconds) the call drops and the fax fails. Error messages shown are as follows: Sender: The call dropped prematurely Receiver: Disconnected after permitted retries Note that when not using authentication, the FAX will complete as expected. When using chan_sip as the sender to a receiver of chan_pjsip, the FAX will also complete as expected with authentication, but if chan_pjsip is the sender it will fail regardless of whether the recipient is chan_sip or chan_pjsip. The problem is caused by duplication of a framehook in res_pjsip_t38 which occurs on the second invite sent out when responding to the auth challenge. Fix: In order to fix this, I added a simple flag to the pjsip session struct that would be raised when the framehook is first attached to prevent duplication. I wouldn't be surprised if there were a better way to do this. Diffs (updated) - /certified/branches/13.1/res/res_pjsip_t38.c 433316 Diff: https://reviewboard.asterisk.org/r/4577/diff/ Testing --- I've duplicated and modified the t38 PJSIP fax test in the testsuite to include authentication. It fails without the patch and passes with the patch. I also tested this locally with my two Asterisk machines in the above scenario. I'll be linking the test after putting it on Gerrit. Thanks, Jonathan Rose -- _ -- 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] 4579: res_pjsip_messaging: Serialize outgoing MESSAGEs
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4579/#review15045 --- Ship it! I'll just say that I enjoy that this is all it took to change the threading model in the module. - Matt Jordan On April 3, 2015, 11:38 a.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4579/ --- (Updated April 3, 2015, 11:38 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24937 https://issues.asterisk.org/jira/browse/ASTERISK-24937 Repository: Asterisk Description --- Because res_pjsip_messaging throws all messages to send into the threadpool, there is no guarantee that consecutive outgoing messages from the same dialplan execution will be sent in the same order. So for instance, if you had the following dialplan: exten = hello,1,NoOp() same = n,SendMessage(hello) same = n,SendMessage(world) It would be expected that the hello message would be sent before the world message. However, it cannot be guaranteed this will happen with the current threadpool usage. The patch on this review introduces a serializer for outgoing MESSAGE requests from Asterisk. This ensures that all MESSAGE requests are sent in the same order that they are processed in the dialplan. Diffs - /branches/13/res/res_pjsip_messaging.c 433838 Diff: https://reviewboard.asterisk.org/r/4579/diff/ Testing --- The bug itself is incredibly difficult to have happen under normal circumstances, but I have confirmed that this patch has not hindered operations any. 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]: non_stasis_bridge_to_stasis_bridge: Update regex for ami events
Hello Ashley Sanders, Corey Farrell, I'd like you to reexamine a change. Please visit https://gerrit.asterisk.org/25 to look at the new patch set (#3). Change subject: non_stasis_bridge_to_stasis_bridge: Update regex for ami events .. non_stasis_bridge_to_stasis_bridge: Update regex for ami events Due to a bug in Asterisk, in some cases, the channel would not be hung up after leaving the bridge upon completing an attended transfer. With that fixed this test can now be expected to pass, so the 'expected-result: False' has been removed. Also a couple of events attempted to match against the wrong regex, so those were modified accordingly. Change-Id: I0a3bcb1a0df7e7bdca02be827288f5f08b5140ce --- M tests/rest_api/external_interaction/attended_transfer/non_stasis_bridge_to_stasis_bridge/test-config.yaml 1 file changed, 4 insertions(+), 7 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/25/25/3 -- To view, visit https://gerrit.asterisk.org/25 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0a3bcb1a0df7e7bdca02be827288f5f08b5140ce Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Kevin Harwell kharw...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Corey Farrell g...@cfware.com Gerrit-Reviewer: John Bigelow jbige...@digium.com -- _ -- 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 a test for PJSIP t38 with authentication based on normal...
Hello Ashley Sanders, I'd like you to reexamine a change. Please visit https://gerrit.asterisk.org/22 to look at the new patch set (#2). Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive .. Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive the call and direct it to an extension that runs receiveFax. As the fax completes, user events are issued to verify that the FAX completed successfully. Addresses a bug detailed in ASTERISK-24933 Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 --- A tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast1/pjsip.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/pjsip.conf A tests/fax/pjsip/t38_with_auth/run-test A tests/fax/pjsip/t38_with_auth/test-config.yaml M tests/fax/pjsip/tests.yaml 7 files changed, 175 insertions(+), 0 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/22/22/2 -- To view, visit https://gerrit.asterisk.org/22 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com -- _ -- 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 a test for PJSIP t38 with authentication based on normal...
Hello Ashley Sanders, I'd like you to reexamine a change. Please visit https://gerrit.asterisk.org/22 to look at the new patch set (#3). Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive .. Add a test for PJSIP t38 with authentication based on normal t38 test The test will start two instances of Asterisk. The first will originate a PJSIP call with authentication to the second using an extension that will run sendFax. The second will receive the call and direct it to an extension that runs receiveFax. As the fax completes, user events are issued to verify that the FAX completed successfully. ASTERISK-24933 Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 --- A tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast1/pjsip.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf A tests/fax/pjsip/t38_with_auth/configs/ast2/pjsip.conf A tests/fax/pjsip/t38_with_auth/run-test A tests/fax/pjsip/t38_with_auth/test-config.yaml M tests/fax/pjsip/tests.yaml 7 files changed, 175 insertions(+), 0 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/22/22/3 -- To view, visit https://gerrit.asterisk.org/22 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 Gerrit-PatchSet: 3 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Matt Jordan mjor...@digium.com -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4580: Fix issue with Makefile that prevents use of ~/.asterisk.makeopts and /etc/asterisk.makeopts
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4580/ --- Review request for Asterisk Developers. Bugs: ASTERISK-13271 https://issues.asterisk.org/jira/browse/ASTERISK-13271 Repository: Asterisk Description --- The Makefile claims that you can set default menuselect options by creating ~/.asterisk.makeopts or /etc/asterisk.makeopts, but they are never read. The rule for menuselect.makeopts is only allowed to run if the active target is 'menuselect', but the menuselect target doesn't depend on menuselect.makeopts. A dot (wildcard character) was added so the rule will be active for the targets that cause it to run: nmenuselect, cmenuselect, and gmenuselect. When I first enabled this it caused ~/.asterisk.makeopts to overwrite ./menuselect.makeopts every time I ran 'make menuselect', so I modified the Makefile to only consider the file in $HOME and /etc when ./menuselect.makeopts doesn't exist. Thoughts: * Having two files ($HOME and /etc) at the same time really doesn't make sense to me, seems like it would be better to use only the most local file. * As far as I can tell this has never worked in any currently supported version. I see this as a new for the second time feature, not a bug fix. I'm thinking trunk only. If anyone disagrees let me know. Diffs - /trunk/Makefile 433966 Diff: https://reviewboard.asterisk.org/r/4580/diff/ Testing --- Repeatedly ran 'make menuselect' with and without files in the 3 locations. No one else has tested this. 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
[asterisk-dev] [Code Review] 4585: res_pjsip: Refactor endpt_send_request to include transaction timeout
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4585/ --- Review request for Asterisk Developers, Joshua Colp and Mark Michelson. Repository: Asterisk Description --- This is the first follow-on to https://reviewboard.asterisk.org/r/4572/ and the discussion at http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html This patch pulls the pjsip_endpt_send_request function out of pjproject and into res_pjsip in order to implement transaction timeout capability. Now when the transaction is initiated, an asterisk sched timer is started. If the transaction completes (or pjsip itself times it out) before the timer expires, the timer is cancelled. If the timer expires before the transaction is completed, the transaction is cancelled. Either way, the callback is called with the TIMER event code. The timeout is supplied in the call to ast_sip_send_out_of_dialog_request. If '-1', no timer is started and the transaction will continue until successful completion or pjsip itself cancels it. Diffs - branches/13/res/res_pjsip.c 433967 branches/13/include/asterisk/res_pjsip.h 433967 Diff: https://reviewboard.asterisk.org/r/4585/diff/ Testing --- Tested that both of the pjsip timeout and asterisk timeout scenarios work and clean up properly. All pjsip testsuite tests that worked before the change still work after the change. A new testsuite test will be written when the companion pjsip_options work is done. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 10:06 p.m.) Review request for Asterisk Developers. Changes --- Added configuration variable to iax.sample.conf Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Changes: - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const. - Add general configuration variable `calltokenexpiration` Diffs (updated) - trunk/configs/samples/iax.conf.sample 432806 trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4582: res_pjsip: config option 'timers' can't be set to 'no'
On April 3, 2015, 4:28 p.m., rmudgett wrote: branches/13/res/res_pjsip/pjsip_configuration.c, lines 152-153 https://reviewboard.asterisk.org/r/4582/diff/1/?file=73574#file73574line152 Are the flags exclusive to each other? Maybe SUPPORT is needed to enable the functionality and REQUIRE to make it mandatory. I looked at the pjsip code and they seem to take care of adding the other flags if needed, so I figured to just let them handle it. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/#review15048 --- On April 3, 2015, 2:58 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/ --- (Updated April 3, 2015, 2:58 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24910 https://issues.asterisk.org/jira/browse/ASTERISK-24910 Repository: Asterisk Description --- When setting the configuration option 'timers' equal to 'no' the bit flag was not properly negated. This patch clears all associated flags and only sets the specified one. pjsip will handle any necessary flag combinations. Also went ahead and did similar for the '100rel' option. Diffs - branches/13/res/res_pjsip/pjsip_configuration.c 433966 Diff: https://reviewboard.asterisk.org/r/4582/diff/ Testing --- Made sure the option can now be set to 'no' and that it clears the bit. Also set it to the other values and reloaded to make sure the field was updated correctly. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4589: res_pjsip_t38: Add missing initialization of udptl-far_max_datagram in t38_initialize_session()
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4589/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24928 https://issues.asterisk.org/jira/browse/ASTERISK-24928 Repository: Asterisk Description --- Initialize udptl-far_max_datagram in t38_initialize_session() with a default value or a value provided in pjsip.conf (t38_udptl_maxdatagram). Without this far_max_datagram remains -1 if the remote endpoint does not provide the MediaAttribute T38FaxMaxDatagram in it's SIP INVITE SDP. This will result in the INVITE being rejected. Diffs - /branches/13/res/res_pjsip_t38.c 433967 Diff: https://reviewboard.asterisk.org/r/4589/diff/ Testing --- Thanks, Juergen Spies -- _ -- 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.
Mark Michelson has abandoned this change. Change subject: Add SIP attended transfer for Asterisk 11. .. Abandoned -- To view, visit https://gerrit.asterisk.org/20 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: abandon 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-Reviewer: Mark Michelson mmichel...@digium.com -- _ -- 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.
Mark Michelson has posted comments on this change. Change subject: Add SIP attended transfer for Asterisk 11. .. Patch Set 1: I am abandoning this change in favor of change /c/29/ -- 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-Reviewer: Mark Michelson mmichel...@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] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 9:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
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/#review15049 --- This is still a nuisance warning that doesn't add much value for the effort. - rmudgett On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25728 This change causes bugs. It is supposed to return peer because it increased the ref. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25729 This change is another bug. It is supposed to return the found user. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25730 In this case we have a ref before calling AST_SCHED_DEL and the scheduler item that we just deleted also has a ref. Therefore peer could never be invalid. The scheduler is very old with a flawed API. It doesn't know how to handle callback data that must be released such as ao2 object refs or malloced data. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25732 This is another case of dealing with the scheduler. We are releasing the ref we just tried to give the scheduler that failed to be added. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25733 This NULL check is not needed. peer can never be NULL here without an earlier crash. This function is a scheduler callback. - rmudgett On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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
[asterisk-dev] Change in testsuite[master]: sip_attended_transfer now supports pre-12 Asterisk versions.
Mark Michelson has posted comments on this change. Change subject: sip_attended_transfer now supports pre-12 Asterisk versions. .. Patch Set 1: (1 comment) https://gerrit.asterisk.org/#/c/29/1/tests/channels/SIP/sip_attended_transfer/attended_transfer.py File tests/channels/SIP/sip_attended_transfer/attended_transfer.py: Line 235: if self.bridge_state.bridge2_bridged and self.carol_call_answered: Got a typo here. This should be self.bridge_staet.bridge2_bridged() (note the added parentheses) As it turns out my test runs must have been passing because it just so happened that bridge 2 was actually bridged before Alice's call to Carol had been answered. -- To view, visit https://gerrit.asterisk.org/29 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d Gerrit-PatchSet: 1 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson mmichel...@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
[asterisk-dev] [Code Review] 4586: AMI: Fix improper handling of lines that are exactly 1025 bytes long.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4586/ --- Review request for Asterisk Developers. Bugs: ASTERISK-20524 https://issues.asterisk.org/jira/browse/ASTERISK-20524 Repository: Asterisk Description --- When AMI receives a line that is 1025 bytes long, it sends two error messages. Copy the last byte in the buffer to the first postiion, set the length to 1. Diffs - /branches/11/main/manager.c 433967 Diff: https://reviewboard.asterisk.org/r/4586/diff/ Testing --- Testsuite - tests/manager against Asterisk 13. All passed. Manually tested against 11 using David's script posted to JIRA. Verified a single error is received for lines 1025 or more, lines 1024 or less succeed. 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
[asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/#review15050 --- You probably want to add documentation for the new configuration option to any appropriate place, such as the iax.conf.sample file. - rnewton On April 3, 2015, 9:32 p.m., Y Ateya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 9:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4588/ --- (Updated April 3, 2015, 9:49 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24939 https://issues.asterisk.org/jira/browse/ASTERISK-24939 Repository: Asterisk Description (updated) --- The section 4.1 in call token changes to IAX protocol (http://downloads.asterisk.org/pub/security/IAX2-security.html): The token timeout will be hard coded at 10 seconds for now. However, it may be made configurable at some point if it seems to be a useful addition In case of lagged network cases (or bad network which required multiple retries) 10 seconds is not enough. Changes: - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const. - Add general configuration variable `calltokenexpiration` Diffs - trunk/channels/chan_iax2.c 432806 Diff: https://reviewboard.asterisk.org/r/4588/diff/ Testing --- Thanks, Y Ateya -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4582: res_pjsip: config option 'timers' can't be set to 'no'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/ --- (Updated April 3, 2015, 5:25 p.m.) Review request for Asterisk Developers. Changes --- Addressed feedback. Bugs: ASTERISK-24910 https://issues.asterisk.org/jira/browse/ASTERISK-24910 Repository: Asterisk Description --- When setting the configuration option 'timers' equal to 'no' the bit flag was not properly negated. This patch clears all associated flags and only sets the specified one. pjsip will handle any necessary flag combinations. Also went ahead and did similar for the '100rel' option. Diffs (updated) - branches/13/res/res_pjsip/pjsip_configuration.c 433966 branches/13/res/res_pjsip.c 433966 Diff: https://reviewboard.asterisk.org/r/4582/diff/ Testing --- Made sure the option can now be set to 'no' and that it clears the bit. Also set it to the other values and reloaded to make sure the field was updated correctly. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] Change in testsuite[master]: sip_attended_transfer now supports pre-12 Asterisk versions.
Mark Michelson has uploaded a new change for review.
https://gerrit.asterisk.org/29
Change subject: sip_attended_transfer now supports pre-12 Asterisk versions.
..
sip_attended_transfer now supports pre-12 Asterisk versions.
The sip_attended transfer test was recently rewritten to prevent it from
bouncing during automatic test runs. The rewrite attempted to break into
two tests in an attempt to separate the logic of different versions from
one another.
Ashley pointed out on my original Asterisk 11 version of the patch that
there are only small difference between the Asterisk 11 and 12 versions
of the test, resulting in a lot of repeated boilerplate code that could
otherwise be avoided.
This change alters the 12+ specific test by separating the bridge logic
for different versions into their own classes. I have verified that the
test passes using both Asterisk 11 and Asterisk 13.
Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d
---
M tests/channels/SIP/sip_attended_transfer/attended_transfer.py
M tests/channels/SIP/sip_attended_transfer/test-config.yaml
2 files changed, 125 insertions(+), 45 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/29/29/1
diff --git a/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
b/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
index cb133f3..ef9f314 100644
--- a/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
+++ b/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
@@ -8,7 +8,11 @@
import logging
import pjsua as pj
+import sys
+sys.path.append('lib/python/asterisk')
+
+from version import AsteriskVersion
from twisted.internet import reactor
LOGGER = logging.getLogger(__name__)
@@ -52,8 +56,8 @@
reactor.callFromThread(self.on_answered)
-class BridgeState(object):
-'''Object for tracking state of a bridge
+class BridgeTwelve(object):
+'''Object for tracking attributes of an Asterisk 12+ bridge
The main data the test cares about is the bridge's unique id and whether
two
channels have been bridged together by the bridge.
@@ -61,6 +65,118 @@
def __init__(self):
self.unique_id = None
self.bridged = False
+
+
+class BridgeStateTwelve(object):
+'''Tracker of Bridge State for Asterisk 12+
+
+Since Asterisk 12+ has the concept of Bridge objects, this tracks the
+bridges by detecting when they are created. Once bridges are created, we
+determine that channels are bridged when BridgeEnter events indicate that
+two channels are present.
+'''
+def __init__(self, test_object, controller, ami):
+self.test_object = test_object
+self.controller = controller
+self.ami = ami
+self.bridge1 = BridgeTwelve()
+self.bridge2 = BridgeTwelve()
+
+self.ami.registerEvent('BridgeCreate', self.bridge_create)
+self.ami.registerEvent('BridgeEnter', self.bridge_enter)
+
+def bridge_create(self, ami, event):
+if not self.bridge1.unique_id:
+self.bridge1.unique_id = event.get('bridgeuniqueid')
+elif not self.bridge2.unique_id:
+self.bridge2.unique_id = event.get('bridgeuniqueid')
+else:
+LOGGER.error(Unexpected third bridge created)
+self.test_object.set_passed(False)
+self.test_object.stop_reactor()
+
+def bridge_enter(self, ami, event):
+if (event.get('bridgeuniqueid') == self.bridge1.unique_id and
+event.get('bridgenumchannels') == '2'):
+self.bridge1.bridged = True
+if self.controller.state == BOB_CALLED:
+self.controller.call_carol()
+elif self.controller.state == TRANSFERRED:
+self.controller.hangup_calls()
+else:
+LOGGER.error(Unexpected BridgeEnter event)
+self.test_object.set_passed(False)
+self.test_object.stop_reactor()
+elif (event.get('bridgeuniqueid') == self.bridge2.unique_id and
+ event.get('bridgenumchannels') == '2'):
+self.bridge2.bridged = True
+if self.controller.state == CAROL_CALLED:
+self.controller.transfer_call()
+elif self.controller.state == TRANSFERRED:
+self.controller.hangup_calls()
+else:
+LOGGER.error(Unexpected BridgeEnter event)
+self.test_object.set_passed(False)
+self.test_object.stop_reactor()
+
+def bridge1_bridged(self):
+return self.bridge1.bridged
+
+def bridge2_bridged(self):
+return self.bridge2.bridged
+
+
+class BridgeStateEleven(object):
+'''Tracker of bridge state for Asterisk 11-
+
+Since in Asterisk versions prior to 12, there are no bridge objects, the
+only way we can track the state of bridges in Asterisk is via Bridge
[asterisk-dev] Change in testsuite[master]: sip_attended_transfer now supports pre-12 Asterisk versions.
Mark Michelson has uploaded a new patch set (#2). Change subject: sip_attended_transfer now supports pre-12 Asterisk versions. .. sip_attended_transfer now supports pre-12 Asterisk versions. The sip_attended transfer test was recently rewritten to prevent it from bouncing during automatic test runs. The rewrite attempted to break into two tests in an attempt to separate the logic of different versions from one another. Ashley pointed out on my original Asterisk 11 version of the patch that there are only small difference between the Asterisk 11 and 12 versions of the test, resulting in a lot of repeated boilerplate code that could otherwise be avoided. This change alters the 12+ specific test by separating the bridge logic for different versions into their own classes. I have verified that the test passes using both Asterisk 11 and Asterisk 13. Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d Fix a typo where a function call was evaluated as a boolean. Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d --- M tests/channels/SIP/sip_attended_transfer/attended_transfer.py M tests/channels/SIP/sip_attended_transfer/test-config.yaml 2 files changed, 125 insertions(+), 45 deletions(-) git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/29/29/2 -- To view, visit https://gerrit.asterisk.org/29 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson mmichel...@digium.com Gerrit-Reviewer: Mark Michelson mmichel...@digium.com -- _ -- 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/ --- (Updated April 3, 2015, 11:24 p.m.) Review request for Asterisk Developers. Changes --- Clarify Purpose of this Review entry 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 (updated) --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 3, 2015, 11:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 11:24 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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
[asterisk-dev] Change in testsuite[master]: Add a test for PJSIP t38 with authentication based on normal...
Jonathan Rose has posted comments on this change. Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test .. Patch Set 1: (2 comments) https://gerrit.asterisk.org/#/c/22/1/tests/fax/pjsip/t38_with_auth/run-test File tests/fax/pjsip/t38_with_auth/run-test: Line 6: Should you add your name to the authors list? Now that I actually have changes in the code? Sure. Line 26:event_count = 0 If you aren't using this class as a singleton, you don't need to have these Probably not the best course of action to change anything based on what you see in this file. It was just a direct copy of something slightly modified from something that was written in 2011 before we revamped a bunch of the testsuite stuff. -- To view, visit https://gerrit.asterisk.org/22 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88 Gerrit-PatchSet: 1 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Jonathan Rose jr...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Jonathan Rose jr...@digium.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
[asterisk-dev] [Code Review] 4587: pjsip_options: Add qualify_timeout processing and eventing
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4587/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This is the second follow-on to https://reviewboard.asterisk.org/r/4572/ and the discussion at http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html The basic issues are that changes in contact status don't cause events to be emitted for the associated endpoint. Only dynamic contact add/delete actions update the endpoint. Also, the qualify timeout is fixed by pjsip at 32 seconds which is a long time. This patch makes use of the new transaction timeout feature in r4585 and provides the following capabilities... 1. A new aor/contact variable 'qualify_timeout' has been added that allows the user to specify the maximum time in milliseconds to wait for a response to an OPTIONS mesasge. The default is 3000ms. When the timer expires, the contact is marked unavailable. 2. Contact status changes are now propagated up to the endpoint as follows... When any contact is 'Available', the endpoint is marked as 'Reachable'. When all contacts are 'Unavailable', the endpoint is marked as 'Unreachable'. The existing endpoint events are generated appropriately. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433967 branches/13/res/res_pjsip/pjsip_configuration.c 433967 branches/13/res/res_pjsip/location.c 433967 branches/13/res/res_pjsip.c 433967 branches/13/main/endpoints.c 433967 branches/13/include/asterisk/res_pjsip.h 433967 branches/13/include/asterisk/endpoints.h 433967 branches/13/contrib/ast-db-manage/config/versions/2256a84ca226_add_pjsip_qualify_timeout.py PRE-CREATION branches/13/configs/samples/pjsip.conf.sample 433967 Diff: https://reviewboard.asterisk.org/r/4587/diff/ Testing --- Existing tests are unchanged. I'm working on new testsuite tests to check the new functionality. Thanks, George Joseph -- _ -- 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] 4582: res_pjsip: config option 'timers' can't be set to 'no'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/#review15048 --- branches/13/res/res_pjsip/pjsip_configuration.c https://reviewboard.asterisk.org/r/4582/#comment25723 This could be flags = ~(SUPPORT | REQUIRE) branches/13/res/res_pjsip/pjsip_configuration.c https://reviewboard.asterisk.org/r/4582/#comment25726 Are the flags exclusive to each other? Maybe SUPPORT is needed to enable the functionality and REQUIRE to make it mandatory. branches/13/res/res_pjsip/pjsip_configuration.c https://reviewboard.asterisk.org/r/4582/#comment25724 Idem branches/13/res/res_pjsip/pjsip_configuration.c https://reviewboard.asterisk.org/r/4582/#comment25727 Are the flags exclusive to each other? Maybe SUPPORT is needed to enable the functionality and REQUIRE to make it mandatory. Similar for ALWAYS. branches/13/res/res_pjsip/pjsip_configuration.c https://reviewboard.asterisk.org/r/4582/#comment25725 The documentation for this enum value is needs updating in res_pjsip.c. It is documented as force instead of always. Maybe add force here as an alias of always since it has been documented that way. - rmudgett On April 3, 2015, 2:58 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/ --- (Updated April 3, 2015, 2:58 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24910 https://issues.asterisk.org/jira/browse/ASTERISK-24910 Repository: Asterisk Description --- When setting the configuration option 'timers' equal to 'no' the bit flag was not properly negated. This patch clears all associated flags and only sets the specified one. pjsip will handle any necessary flag combinations. Also went ahead and did similar for the '100rel' option. Diffs - branches/13/res/res_pjsip/pjsip_configuration.c 433966 Diff: https://reviewboard.asterisk.org/r/4582/diff/ Testing --- Made sure the option can now be set to 'no' and that it clears the bit. Also set it to the other values and reloaded to make sure the field was updated correctly. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Stupid me. If should actually also check if ao2_ref did not return an error (-1). On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 11131-11136 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line11131 This is another case of dealing with the scheduler. We are releasing the ref we just tried to give the scheduler that failed to be added. made it a conscious choice choice to discard the pointer using (void)peer_unref(peer); Maybe time to update Scheduler :-) On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2021-2025 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2021 This change is another bug. It is supposed to return the found user. Guess i should not be doing any of this stuff at 3 Am i guess. Difficult to make a point, whilst making mistakes like these. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 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 (updated) - /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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 4:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. Diederik de Groot wrote: By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. Those findings were not real bugs. See my review of those specific findings. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Diederik de Groot wrote: Stupid me. If should actually also check if ao2_ref did not return an error (-1). rmudgett wrote: The return value of ao2_ref is rarely used. When it is it is to get the number of refs the object had before the call. The -1 error return value is practically useless. That value means a programming error because the object is invalid and you get bad magic number messages logged, assertion failures, and very likely crashes. Ok, point taken. So that would mean that ao2_ref will be the biggest cause of -Wunused-value warning, even if we changed everything else, because it's return is rarely used :-) I still think it is valid to NULL a pointer that might/might not vanish in the future (as with ao2_ref(xx, -1)). Same goes for example for ast_free, which does not null the pointer afterwards (or did i overlook something ?. It makes NULL pointer checks after such a free or unref a little bit pointless. I know everything is working and asterisk-11 and asterisk-13 are pretty stable as far as i can tell, so not looking for extra work for any of us, just want to finish this point and move on. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4582: res_pjsip: config option 'timers' can't be set to 'no'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/#review15056 --- Ship it! Ship It! - rmudgett On April 3, 2015, 5:52 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/ --- (Updated April 3, 2015, 5:52 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24910 https://issues.asterisk.org/jira/browse/ASTERISK-24910 Repository: Asterisk Description --- When setting the configuration option 'timers' equal to 'no' the bit flag was not properly negated. This patch clears all associated flags and only sets the specified one. pjsip will handle any necessary flag combinations. Also went ahead and did similar for the '100rel' option. Diffs - branches/13/res/res_pjsip/pjsip_configuration.c 433966 branches/13/res/res_pjsip.c 433966 Diff: https://reviewboard.asterisk.org/r/4582/diff/ Testing --- Made sure the option can now be set to 'no' and that it clears the bit. Also set it to the other values and reloaded to make sure the field was updated correctly. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 5:40 p.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Diederik de Groot wrote: Stupid me. If should actually also check if ao2_ref did not return an error (-1). The return value of ao2_ref is rarely used. When it is it is to get the number of refs the object had before the call. The -1 error return value is practically useless. That value means a programming error because the object is invalid and you get bad magic number messages logged, assertion failures, and very likely crashes. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 3, 2015, 6:15 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 6: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:-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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4582: res_pjsip: config option 'timers' can't be set to 'no'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4582/ --- (Updated April 3, 2015, 5:52 p.m.) Review request for Asterisk Developers. Changes --- Added description in the enumeration doc field for forced stating that it is an alias of always. Bugs: ASTERISK-24910 https://issues.asterisk.org/jira/browse/ASTERISK-24910 Repository: Asterisk Description --- When setting the configuration option 'timers' equal to 'no' the bit flag was not properly negated. This patch clears all associated flags and only sets the specified one. pjsip will handle any necessary flag combinations. Also went ahead and did similar for the '100rel' option. Diffs (updated) - branches/13/res/res_pjsip/pjsip_configuration.c 433966 branches/13/res/res_pjsip.c 433966 Diff: https://reviewboard.asterisk.org/r/4582/diff/ Testing --- Made sure the option can now be set to 'no' and that it clears the bit. Also set it to the other values and reloaded to make sure the field was updated correctly. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] Change in testsuite[master]: stasis: set a channel variable on websocket disconnect error
Ashley Sanders has posted comments on this change. Change subject: stasis: set a channel variable on websocket disconnect error .. Patch Set 2: (1 comment) https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/ari_client.py File tests/rest_api/applications/stasisstatus/ari_client.py: Line 270: resource -- The resource to use to construct the endpoint : for the ARI request. (optional) : (default None.) If no value is provided, : resource will automatically be generated in : the form: 'LOCAL/app_name@Acme'. To make this more generically useful, I'd have the caller pass in the endpo Good catch. Yes, I 100% agree with you. This should be refactored to live in the builder. -- 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: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Ashley Sanders asand...@digium.com Gerrit-Reviewer: Mark Michelson mmichel...@digium.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] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. Diederik de Groot wrote: By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. rmudgett wrote: Those findings were not real bugs. See my review of those specific findings. Agreed ! So if anybody else thinks we should drop this warning let me know, and we'll drop it. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 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 --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. 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] 4577: res_pjsip_t38: T38 fax fails when using authentication with PJSIP sender
On April 2, 2015, 6:13 p.m., Kevin Harwell wrote: It is probably always the case that framehooks should not be attached twice. If this is true then it might be better to add a check in 'ast_framehook_attach' that first makes sure the hook is not already in the list. If so don't add it again. rmudgett wrote: It is specific framehooks that cannot be attached twice. It is not a general requirement/property of the framehook concept. Just curious, but what's a case where a framehook should be attached twice? - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/#review15034 --- On April 2, 2015, 2:08 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/ --- (Updated April 2, 2015, 2:08 p.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24933 https://issues.asterisk.org/jira/browse/ASTERISK-24933 Repository: Asterisk Description --- Description: Two Asterisk boxes each have the other as endpoints with authentication set. First Asterisk box originates a call to the second using the PJSIP endpoint. The first Asterisk box uses an extension with sendfax, the second uses an extension with receivefax. The session starts fairly normally, but resolution never appears in fax show session output. After a while (~25 seconds) the call drops and the fax fails. Error messages shown are as follows: Sender: The call dropped prematurely Receiver: Disconnected after permitted retries Note that when not using authentication, the FAX will complete as expected. When using chan_sip as the sender to a receiver of chan_pjsip, the FAX will also complete as expected with authentication, but if chan_pjsip is the sender it will fail regardless of whether the recipient is chan_sip or chan_pjsip. The problem is caused by duplication of a framehook in res_pjsip_t38 which occurs on the second invite sent out when responding to the auth challenge. Fix: In order to fix this, I added a simple flag to the pjsip session struct that would be raised when the framehook is first attached to prevent duplication. I wouldn't be surprised if there were a better way to do this. Diffs - /certified/branches/13.1/res/res_pjsip_t38.c 433316 /certified/branches/13.1/include/asterisk/res_pjsip_session.h 433316 Diff: https://reviewboard.asterisk.org/r/4577/diff/ Testing --- I've duplicated and modified the t38 PJSIP fax test in the testsuite to include authentication. It fails without the patch and passes with the patch. I also tested this locally with my two Asterisk machines in the above scenario. I'll be linking the test after putting it on Gerrit. Thanks, Jonathan Rose -- _ -- 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] 4577: res_pjsip_t38: T38 fax fails when using authentication with PJSIP sender
On April 2, 2015, 6:13 p.m., Kevin Harwell wrote: It is probably always the case that framehooks should not be attached twice. If this is true then it might be better to add a check in 'ast_framehook_attach' that first makes sure the hook is not already in the list. If so don't add it again. rmudgett wrote: It is specific framehooks that cannot be attached twice. It is not a general requirement/property of the framehook concept. Kevin Harwell wrote: Just curious, but what's a case where a framehook should be attached twice? Jonathan Rose wrote: You could have multiple mixmonitors attached by separate parts of the dialplan saving to different files as an example. Well, that might be audiohooks instead of framehooks actually... but it's the same concept. - Jonathan --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/#review15034 --- On April 2, 2015, 2:08 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/ --- (Updated April 2, 2015, 2:08 p.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24933 https://issues.asterisk.org/jira/browse/ASTERISK-24933 Repository: Asterisk Description --- Description: Two Asterisk boxes each have the other as endpoints with authentication set. First Asterisk box originates a call to the second using the PJSIP endpoint. The first Asterisk box uses an extension with sendfax, the second uses an extension with receivefax. The session starts fairly normally, but resolution never appears in fax show session output. After a while (~25 seconds) the call drops and the fax fails. Error messages shown are as follows: Sender: The call dropped prematurely Receiver: Disconnected after permitted retries Note that when not using authentication, the FAX will complete as expected. When using chan_sip as the sender to a receiver of chan_pjsip, the FAX will also complete as expected with authentication, but if chan_pjsip is the sender it will fail regardless of whether the recipient is chan_sip or chan_pjsip. The problem is caused by duplication of a framehook in res_pjsip_t38 which occurs on the second invite sent out when responding to the auth challenge. Fix: In order to fix this, I added a simple flag to the pjsip session struct that would be raised when the framehook is first attached to prevent duplication. I wouldn't be surprised if there were a better way to do this. Diffs - /certified/branches/13.1/res/res_pjsip_t38.c 433316 /certified/branches/13.1/include/asterisk/res_pjsip_session.h 433316 Diff: https://reviewboard.asterisk.org/r/4577/diff/ Testing --- I've duplicated and modified the t38 PJSIP fax test in the testsuite to include authentication. It fails without the patch and passes with the patch. I also tested this locally with my two Asterisk machines in the above scenario. I'll be linking the test after putting it on Gerrit. Thanks, Jonathan Rose -- _ -- 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] 4577: res_pjsip_t38: T38 fax fails when using authentication with PJSIP sender
On April 2, 2015, 6:13 p.m., Kevin Harwell wrote: It is probably always the case that framehooks should not be attached twice. If this is true then it might be better to add a check in 'ast_framehook_attach' that first makes sure the hook is not already in the list. If so don't add it again. rmudgett wrote: It is specific framehooks that cannot be attached twice. It is not a general requirement/property of the framehook concept. Kevin Harwell wrote: Just curious, but what's a case where a framehook should be attached twice? You could have multiple mixmonitors attached by separate parts of the dialplan saving to different files as an example. - Jonathan --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/#review15034 --- On April 2, 2015, 2:08 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4577/ --- (Updated April 2, 2015, 2:08 p.m.) Review request for Asterisk Developers and Joshua Colp. Bugs: ASTERISK-24933 https://issues.asterisk.org/jira/browse/ASTERISK-24933 Repository: Asterisk Description --- Description: Two Asterisk boxes each have the other as endpoints with authentication set. First Asterisk box originates a call to the second using the PJSIP endpoint. The first Asterisk box uses an extension with sendfax, the second uses an extension with receivefax. The session starts fairly normally, but resolution never appears in fax show session output. After a while (~25 seconds) the call drops and the fax fails. Error messages shown are as follows: Sender: The call dropped prematurely Receiver: Disconnected after permitted retries Note that when not using authentication, the FAX will complete as expected. When using chan_sip as the sender to a receiver of chan_pjsip, the FAX will also complete as expected with authentication, but if chan_pjsip is the sender it will fail regardless of whether the recipient is chan_sip or chan_pjsip. The problem is caused by duplication of a framehook in res_pjsip_t38 which occurs on the second invite sent out when responding to the auth challenge. Fix: In order to fix this, I added a simple flag to the pjsip session struct that would be raised when the framehook is first attached to prevent duplication. I wouldn't be surprised if there were a better way to do this. Diffs - /certified/branches/13.1/res/res_pjsip_t38.c 433316 /certified/branches/13.1/include/asterisk/res_pjsip_session.h 433316 Diff: https://reviewboard.asterisk.org/r/4577/diff/ Testing --- I've duplicated and modified the t38 PJSIP fax test in the testsuite to include authentication. It fails without the patch and passes with the patch. I also tested this locally with my two Asterisk machines in the above scenario. I'll be linking the test after putting it on Gerrit. Thanks, Jonathan Rose -- _ -- 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
