Re: [asterisk-dev] [Code Review] 4588: IAX make calltoken expiration time configurable

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4588/#review15152
---

Ship it!


Other than having this get documented in the CHANGES file, this looks good to 
go. Since that's relatively minor, I'll Ship It.

- Matt Jordan


On April 9, 2015, 9:21 a.m., Y Ateya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4588/
> ---
> 
> (Updated April 9, 2015, 9:21 a.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.
> 
> Changes:
>  - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const.
>  - Add general configuration variable `calltokenexpiration`
> 
> 
> Diffs
> -
> 
>   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] 4587: pjsip_options: Add qualify_timeout processing and eventing

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4587/#review15151
---



branches/13/configs/samples/pjsip.conf.sample


We need to pick a measurement and stick with it. The qualify_frequency 
option is in seconds. The qualify_timeout option is in milliseconds. As a user 
that would be confusing.



branches/13/res/res_pjsip/pjsip_configuration.c


This is abuse of RAII_VAR :P it's not needed.



branches/13/res/res_pjsip/pjsip_configuration.c


You have a leak here. ao2_iterator isn't destroyed.



branches/13/res/res_pjsip/pjsip_options.c


You *CAN NOT* do this. Sorcery objects are immutable. You have to create a 
new one and then update using it.



branches/13/res/res_pjsip/pjsip_options.c


Same here.


- Joshua Colp


On April 7, 2015, 11:54 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4587/
> ---
> 
> (Updated April 7, 2015, 11:54 p.m.)
> 
> 
> 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.
> 
> Tested-by: Dmitriy Serov
> 
> 
> 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

[asterisk-dev] Change in testsuite[master]: Testsuite: Caller & callee initiated local attended transfer...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: Testsuite: Caller & callee initiated local attended transfers 
to application.
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.asterisk.org/34
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I61b70f83013b8d37de3fe1eea53c8dd2d026d2e0
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: John Bigelow 
Gerrit-Reviewer: Matt Jordan 
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] 4391: Add blank line between headers and output for Command action response

[Corey Farrell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15168
---



/trunk/main/cli.c


These two lines should be replaced with 'goto done;'.  This way we will 
return RESULT_FAILURE.


- Corey Farrell


On April 9, 2015, 1:05 a.m., gareth wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> ---
> 
> (Updated April 9, 2015, 1:05 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 434448 
>   /trunk/main/cli.c 434448 
>   /trunk/include/asterisk/manager.h 434448 
>   /trunk/UPGRADE.txt 434448 
> 
> 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]: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.asterisk.org/35
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5553af9cbfdc1d68110036eaadcfe5db3570928f
Gerrit-PatchSet: 3
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell 
Gerrit-Reviewer: Anonymous Coward #119
Gerrit-Reviewer: Matt Jordan 
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]: pjsip: Add test for OPTIONS requests received in-dialog.

[Joshua Colp (Code Review)]

Joshua Colp has uploaded a new change for review.

  https://gerrit.asterisk.org/37

Change subject: pjsip: Add test for OPTIONS requests received in-dialog.
..

pjsip: Add test for OPTIONS requests received in-dialog.

This test establishes a session with Asterisk. Once established an
OPTIONS request is sent in-dialog. Asterisk is expected to respond
with a 200 OK. If this does not occur the test fails.

ASTERISK-24862 #close
Reported by: yaron nahum

Change-Id: I137c66e4faad5212040f6aded6be2aac20fc473d
---
A tests/channels/pjsip/in_dialog_options/configs/ast1/extensions.conf
A tests/channels/pjsip/in_dialog_options/configs/ast1/pjsip.conf
A tests/channels/pjsip/in_dialog_options/sipp/invite_with_options.xml
A tests/channels/pjsip/in_dialog_options/test-config.yaml
M tests/channels/pjsip/tests.yaml
5 files changed, 168 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/37/37/1

diff --git 
a/tests/channels/pjsip/in_dialog_options/configs/ast1/extensions.conf 
b/tests/channels/pjsip/in_dialog_options/configs/ast1/extensions.conf
new file mode 100644
index 000..711c6cb
--- /dev/null
+++ b/tests/channels/pjsip/in_dialog_options/configs/ast1/extensions.conf
@@ -0,0 +1,4 @@
+[default]
+exten => playback,1,Answer()
+same  =>  n,Playback(demo-congrats)
+same  =>  n,Hangup()
diff --git a/tests/channels/pjsip/in_dialog_options/configs/ast1/pjsip.conf 
b/tests/channels/pjsip/in_dialog_options/configs/ast1/pjsip.conf
new file mode 100644
index 000..79da0da
--- /dev/null
+++ b/tests/channels/pjsip/in_dialog_options/configs/ast1/pjsip.conf
@@ -0,0 +1,15 @@
+[local-transport-template](!)
+type=transport
+bind=127.0.0.1
+
+[local-transport-udp](local-transport-template)
+protocol=udp
+
+[endpoint-template-ipv4](!)
+type=endpoint
+context=default
+allow=!all,ulaw,alaw
+media_address=127.0.0.1
+
+[call](endpoint-template-ipv4)
+transport=local-transport-udp
diff --git 
a/tests/channels/pjsip/in_dialog_options/sipp/invite_with_options.xml 
b/tests/channels/pjsip/in_dialog_options/sipp/invite_with_options.xml
new file mode 100644
index 000..18c3a8a
--- /dev/null
+++ b/tests/channels/pjsip/in_dialog_options/sipp/invite_with_options.xml
@@ -0,0 +1,120 @@
+
+
+
+
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+
+
diff --git a/tests/channels/pjsip/in_dialog_options/test-config.yaml 
b/tests/channels/pjsip/in_dialog_options/test-config.yaml
new file mode 100644
index 000..4e5692d
--- /dev/null
+++ b/tests/channels/pjsip/in_dialog_options/test-config.yaml
@@ -0,0 +1,27 @@
+testinfo:
+summary: 'Tests handling of an in-dialog OPTIONS'
+description: |
+This test runs a SIPp scenario which establishes a session with 
Asterisk.
+Once the session has been established an OPTIONS request is sent 
in-dialog.
+Asterisk is expected to respond with a 200 OK. If this does not occur 
then
+the test fails.
+
+test-modules:
+test-object:
+config-section: test-object-config
+typename: 'sipp.SIPpTestCase'
+
+test-object-config:
+fail-on-any: False
+test-iterations:
+-
+scenarios:
+- { 'key-args': {'scenario': 'invite_with_options.xml', '-i': 
'127.0.0.1', '-p': '5061', '-d': '5000', '-s': 'call'} }
+
+properties:
+minversion: '13.4.0'
+dependencies:
+- app : 'sipp'
+- asterisk : 'res_pjsip'
+tags:
+- pjsip
diff --git a/tests/channels/pjsip/tests.yaml b/tests/channels/pjsip/tests.yaml
index 54b9d35..ddc655a 100644
--- a/tests/channels/pjsip/tests.yaml
+++ b/tests/channels/pjsip/tests.yaml
@@ -35,3 +35,5 @@
 - test: 'keep_alive'
 - test: 'endpoint_identify'
 - test: 'rpid_immediate'
+- test: 'in_dialog_options'
+

-- 
To view, visit https://gerrit.asterisk.org/37
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I137c66e4faad5212040f6aded6be2aac20fc473d
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp 

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

[asterisk-dev] Change in testsuite[master]: rest_api/applications/stasisstatus: Make run-test executable

[Matt Jordan (Code Review)]

Matt Jordan has uploaded a new change for review.

  https://gerrit.asterisk.org/36

Change subject: rest_api/applications/stasisstatus: Make run-test executable
..

rest_api/applications/stasisstatus: Make run-test executable

If it isn't executable, it can't run!

Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
---
M tests/rest_api/applications/stasisstatus/run-test
1 file changed, 0 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/36/36/1

diff --git a/tests/rest_api/applications/stasisstatus/run-test 
b/tests/rest_api/applications/stasisstatus/run-test
old mode 100644
new mode 100755

-- 
To view, visit https://gerrit.asterisk.org/36
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan 

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4604: loader/main: Don't set ast_fully_booted until deferred reloads are processed

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4604/#review15153
---

Ship it!


Ship It!

- Mark Michelson


On April 9, 2015, 2:57 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4604/
> ---
> 
> (Updated April 9, 2015, 2:57 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Until we have a true module management facility it's sometimes necessary for 
> one module to force a reload on another before its own load is complete.  If 
> Asterisk isn't fully booted yet, these reloads are deferred.  The problem is 
> that asterisk reports fully booted before processing the deferred reloads 
> which means Asterisk really isn't quite ready when it says it is.
> 
> This patch moves the report of fully booted after the processing of the 
> deferred reloads is complete.
> 
> 
> Diffs
> -
> 
>   branches/13/main/loader.c 434447 
>   branches/13/main/asterisk.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4604/diff/
> 
> 
> Testing
> ---
> 
> Since the pjsip stack has the most number of related modules, I ran the 
> channels/pjsip testsuite to make sure there aren't any issues.  All tests 
> passed.
> 
> 
> 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] 4608: res_pjsip_phoneprov_provider: Fix reference leak on unload

[George Joseph]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4608/
---

(Updated April 9, 2015, 1:57 p.m.)


Review request for Asterisk Developers and Corey Farrell.


Changes
---

Addressed Corey's findings.


Bugs: ASTERISK-24935
https://issues.asterisk.org/jira/browse/ASTERISK-24935


Repository: Asterisk


Description
---

res_pjsip_phoneprov_provider was leaking references to phoneprov objects due to 
a missing OBJ_NODATA in an ao2_callback in load_users().  Rather than adding 
the OBJ_NODATA, I changed load_users to use a more straightforward 
ao2_iterator.  This plugged the leak but exposed an unload order issue between 
res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip.

res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then res_pjsip. 
 Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery instance, when it 
unloads, it's objects are still in the sorcery instance.  When res_pjsip 
unloads, it destroys all its objects including res_pjsip_phoneprov_provider's.  
The phoneprov destructor then attempts to unregister the extension from 
res_phoneprov but because res_phoneprov is already cleaned up, its users 
container is gone and we get a FRACK.

Simple solution, check for the NULL users container before attempting to remove 
the entry. Duh.


Diffs (updated)
-

  branches/13/res/res_pjsip_phoneprov_provider.c 434447 
  branches/13/res/res_phoneprov.c 434447 

Diff: https://reviewboard.asterisk.org/r/4608/diff/


Testing
---

Ran tests/res_phoneprov/res_phoneprov_provider.  No leaks in 
res_pjsip_phoneprov_provider and no FRACKs.


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] 4391: Add blank line between headers and output for Command action response

[Matt Jordan]

> On April 3, 2015, 5:37 a.m., Corey Farrell wrote:
> > /trunk/main/manager.c, line 4873
> > 
> >
> > 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.
> 
> gareth wrote:
> If the caller is executing any of the ' show ...' commands then 
> they likely care about the output. In this case, I think it would be better 
> to provide a more descriptive error message to the caller so they can detect 
> if the command was executed.
> 
> Yes, ast_cli_commmand_full should indicate whether the command failed, I 
> will modify it so that an Error response can sent if the command fails. There 
> don't appear to be any callers of that function who check the return code.
> 
> Corey Farrell wrote:
> I do not feel this issue is resolved.  If a command has side-effects, we 
> should respond with Success/Failure based on if the command ran - regardless 
> of our (in)ability to process output.  Since there is no way to determine if 
> a CLI command is meant to retrieve information or manipulate it, we have to 
> assume that it is manipulating something.  So I agree that descriptive error 
> messages are useful, we need to avoid lying to the caller by claiming that 
> the command failed to run if it did run.
> 
> For times where people are just retrieving information, couldn't we 
> respond with success, put the error in 'Message:', and provide no 'Output:' 
> headers?  This way if you care about the output, you can detect the lack of 
> output.

This is a tricky one, but I think I agree with Corey on this.

We will already send an Error with a Message value if the command was executed 
but returned a failure response code. The fact that we executed the command 
successfully but had an internal error that precluded showing the response 
feels like it should be different from conditions where we either never ran the 
command at all.

How about changing the astman_send_error to the Message: that is sent with a 
status of Error? In those cases, there wouldn't be any response that follows - 
since we can't send it - but there is still an overall indication that the 
command succeeded (sorta).


- Matt


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
---


On April 9, 2015, 12:05 a.m., gareth wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> ---
> 
> (Updated April 9, 2015, 12:05 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\

Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality

[Diederik de Groot]

> On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
> > I don't understand the purpose of this warning. I tried searching for 
> > details about this warning flag on the internet and came up empty, so I 
> > can't find any documentation that explains what type of error this check is 
> > supposed to help avoid.
> > 
> > It appears that the warning is intended to get rid of "extra" parentheses 
> > where they are unnecessary. The problem is that I don't see anything wrong 
> > with having them there, especially in macro definitions.
> 
> Diederik de Groot wrote:
> Ok you know that to prevent an unintendet assignment where a comparisson 
> was indended compilers request you to write
> 
> /* comparisson */
> if (x == 1) {
> }
> but
> 
> /* assignment => most modern compilers will complain*/
> if (x = 1) {   /* <= warning raised*/ 
> }
> 
> so you are forced to write
> if ((x = 1)) {
> }
> instead, to guarantee that you meant to assign a value here.
> 
> But most compilers will not complain if you write the first comparison 
> between double parantheses
> if ((x == 1)) {
> }
> Which is perfectly legal, as you pointed out. 
> 
> But a maintainer of the code might later be in doubt as to what you mean. 
> Was an assignment was not intended by the original writer (non-obvious).
> 
> clang can be made complain about this (-Wparantheses-equalty or -Wall 
> -Werror) to make sure this last one is not allowed. 
> 
> So this can be considered the reverse of the first warning, where an 
> assignment requires double parantheses.
> 
> rmudgett wrote:
> I think this warning is a backlash from the warning about putting 
> assignments inside tests that you suppress by adding parentheses.
> if ((a = b))
> 
> At any rate this is a very bad warning and should not be used.
> 
> The parentheses in macros are to prevent unexpected precedence operator 
> bindings:
> #define BAD_MACRO(a,b) a + b
> 
> if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf("ERROR unexpected 
> result\n");
>
> 
> Diederik de Groot wrote:
> @rmudgett: I agreed, that's why i was asking in my initial description, 
> if anybody knew of a better way to solve both the macro issue but still 
> satisfy the parantheses warning. Without having to suppress it. Something 
> ugly to fix the macro expansion is to use a double negating comparison or 
> maybe even an inline comparison return TRUE/FALSE. Both of which did not 
> really strike me as particularly nice.
> 
> I can live with suppressing this warning, but it would be nice if 
> somebody knew of a nice way to have it both ways.
>
> 
> rmudgett wrote:
> Please discard this review with predjudice.  I see no benefit to this 
> -Wparentheses-equality option.
> 
> Diederik de Groot wrote:
> WOuld have been nice if this Warning would not react to results coming 
> from macro's (so only plain code), but alas. I guess the AST does not have 
> that last little detail.

I guess we should drop this one. Maybe someone will come up with a nice 
solution in the future. There is a open issue on the clang bug report site 
about this exact warning (currently without a nice solution).


- Diederik


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---


On April 1, 2015, 3:33 a.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4550/
> ---
> 
> (Updated April 1, 2015, 3:33 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
> https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> clang's static analyzer will throw quite a number warnings / errors during 
> compilation, some of which can be very helpfull in finding corner-case bugs.
> 
> clang compiler warning:--dev-mode and -Wparentheses-equality
> 
> include/asterisk/linkedlists.h:450
> ==
> Got rid of the extraneous "()" in the comparison to NULL by negating the 
> comparison
> 
> include/asterisk/vector.h:261
> =
> Removed the extraneous "()". Not particularly happy about this though as they 
> where used to keep this macro encapsulated (Does however not cause any 
> compile issues)
> 
> Another possible solutions would be to double negate the comparison !((elem) 
> != (value)) which would keep everything encapsulated, but does result in a 
> double negative, which is ugly as well.
> 
> main/format_cap.c:467
> =
> Removed the extraneous "()". Not particularl

Re: [asterisk-dev] [Code Review] 4604: loader/main: Don't set ast_fully_booted until deferred reloads are processed

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4604/#review15146
---



branches/13/main/loader.c


Nitpick #1: Since this is static, there's no real reason to assign 0 to it.

Nitpick #2: Even though this one is clear, doxygen comments for globals is 
always nice.


- Matt Jordan


On April 9, 2015, 12:51 a.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4604/
> ---
> 
> (Updated April 9, 2015, 12:51 a.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Until we have a true module management facility it's sometimes necessary for 
> one module to force a reload on another before its own load is complete.  If 
> Asterisk isn't fully booted yet, these reloads are deferred.  The problem is 
> that asterisk reports fully booted before processing the deferred reloads 
> which means Asterisk really isn't quite ready when it says it is.
> 
> This patch moves the report of fully booted after the processing of the 
> deferred reloads is complete.
> 
> 
> Diffs
> -
> 
>   branches/13/main/loader.c 434447 
>   branches/13/main/asterisk.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4604/diff/
> 
> 
> Testing
> ---
> 
> Since the pjsip stack has the most number of related modules, I'm running the 
> channels/pjsip testsuite to make sure there aren't any issues.  So far...none.
> 
> 
> 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

[asterisk-dev] Change in testsuite[master]: Testsuite: Caller & callee initiated local attended transfer...

[John Bigelow (Code Review)]

John Bigelow has uploaded a new patch set (#2).

Change subject: Testsuite: Caller & callee initiated local attended transfers 
to application.
..

Testsuite: Caller & callee initiated local attended transfers to application.

This adds two tests for caller and callee initiated local attended transfers
using PJSIP. The transfer target for both tests is a dialplan application.

ASTERISK-23695 #close

Change-Id: I61b70f83013b8d37de3fe1eea53c8dd2d026d2e0
---
M lib/python/asterisk/phones.py
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/configs/ast1/extensions.conf
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/configs/ast1/pjsip.conf
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/test-config.yaml
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/configs/ast1/extensions.conf
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/configs/ast1/pjsip.conf
A 
tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/test-config.yaml
M tests/channels/pjsip/transfers/attended_transfer/nominal/tests.yaml
8 files changed, 494 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/34/34/2
-- 
To view, visit https://gerrit.asterisk.org/34
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b70f83013b8d37de3fe1eea53c8dd2d026d2e0
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: John Bigelow 
Gerrit-Reviewer: Matt Jordan 

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response

[Corey Farrell]

> On April 3, 2015, 6:37 a.m., Corey Farrell wrote:
> > /trunk/main/manager.c, line 4873
> > 
> >
> > 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.
> 
> gareth wrote:
> If the caller is executing any of the ' show ...' commands then 
> they likely care about the output. In this case, I think it would be better 
> to provide a more descriptive error message to the caller so they can detect 
> if the command was executed.
> 
> Yes, ast_cli_commmand_full should indicate whether the command failed, I 
> will modify it so that an Error response can sent if the command fails. There 
> don't appear to be any callers of that function who check the return code.

I do not feel this issue is resolved.  If a command has side-effects, we should 
respond with Success/Failure based on if the command ran - regardless of our 
(in)ability to process output.  Since there is no way to determine if a CLI 
command is meant to retrieve information or manipulate it, we have to assume 
that it is manipulating something.  So I agree that descriptive error messages 
are useful, we need to avoid lying to the caller by claiming that the command 
failed to run if it did run.

For times where people are just retrieving information, couldn't we respond 
with success, put the error in 'Message:', and provide no 'Output:' headers?  
This way if you care about the output, you can detect the lack of output.


- Corey


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
---


On April 9, 2015, 1:05 a.m., gareth wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> ---
> 
> (Updated April 9, 2015, 1:05 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 434448 
>   /trunk/main/cli.c 434448 
>   /trunk/include/asterisk/manager.h 434448 
>   /trunk/UPGRADE.txt 434448 
> 
> 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
> 
>

-- 
__

Re: [asterisk-dev] [Code Review] 4499: Support in dialog OPTIONS

[yaron nahum]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4499/
---

(Updated April 9, 2015, 10:44 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Bugs: ASTERISK-24862
https://issues.asterisk.org/jira/browse/ASTERISK-24862


Repository: Asterisk


Description
---

Respond to OPTIONS message sent on an existing dialog with 200OK. 
This feature is vital in order to interoperate with some switches that send 
OPTIONS message periodically per active call to make sure it is still alive. 
Not responding would cause the switch to disconnect the call. 
This functionality used to work on the old SIP channel, but was not implemented 
on PJSIP.


Diffs
-

  /trunk/res/res_pjsip_dlg_options.c PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/4499/diff/


Testing
---


Thanks,

yaron nahum

-- 
_
-- 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] 4596: res_pjsip_phoneprov_provider: Fix leaked OBJ_MULTIPLE iterator (2nd try)

[George Joseph]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4596/
---

(Updated April 9, 2015, 11:52 a.m.)


Status
--

This change has been discarded.


Review request for Asterisk Developers and Corey Farrell.


Bugs: ASTERISK-24935
https://issues.asterisk.org/jira/browse/ASTERISK-24935


Repository: Asterisk


Description
---

Original issue:  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.

Unfortunately, this highlighted a module unload order issue where res_phoneprov 
and res_pjsip_phoneprov_provider were unloading before res_pjsip_config_wizard, 
which needed them.

res_pjsip_config_wizard is itself a sorcery wizard so there are some 
complexities to it's load order (it's long story) but I've removed the 
GLOBAL_SYMBOLS flag from res_pjsip_config_wizard so it loads later and unloads 
earlier and also triggered a reload of res_pjsip_phoneprov_provider.  Now they 
load and unload in the correct order.


Diffs
-

  branches/13/res/res_pjsip_phoneprov_provider.c 434423 
  branches/13/res/res_pjsip_config_wizard.c 434423 

Diff: https://reviewboard.asterisk.org/r/4596/diff/


Testing
---

Checked load/unload order and make sure there were no FRACKs on unload.


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

[asterisk-dev] Change in testsuite[master]: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.
..


Patch Set 2:

(1 comment)

https://gerrit.asterisk.org/#/c/35/2/tests/phoneprov/res_phoneprov_pjsip/run-test
File tests/phoneprov/res_phoneprov_pjsip/run-test:

Line 71: 
Extraneous white space


-- 
To view, visit https://gerrit.asterisk.org/35
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5553af9cbfdc1d68110036eaadcfe5db3570928f
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell 
Gerrit-Reviewer: Anonymous Coward #119
Gerrit-Reviewer: Matt Jordan 
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] 4606: chan_sip: make progressinband default to no

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4606/#review15160
---

Ship it!


Ship It!

- Joshua Colp


On April 9, 2015, 5:05 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4606/
> ---
> 
> (Updated April 9, 2015, 5:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24835
> https://issues.asterisk.org/jira/browse/ASTERISK-24835
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> After the "progressinband" value setting of "never" was updated to never send 
> a 183 this separated its use from the "no" value. Since "never" was the 
> default, but most users probably expect "no" this patch updates the default 
> for the "progressinband" setting to "no".
> 
> 
> Diffs
> -
> 
>   branches/13/configs/samples/sip.conf.sample 434508 
>   branches/13/channels/sip/include/sip.h 434508 
>   branches/13/channels/chan_sip.c 434508 
>   branches/13/CHANGES 434508 
> 
> Diff: https://reviewboard.asterisk.org/r/4606/diff/
> 
> 
> Testing
> ---
> 
> Started Asterisk and loaded chan_sip with the new default value for 
> progressinband. Check to make sure that is what it was set to. Changed the 
> setting to the other values and reloaded/checked between each to make sure 
> those got set correctly as well.
> 
> 
> 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] 4609: chan_pjsip/res_pjsip/bridge_softmix/core: Improve translation path choices.

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4609/
---

Review request for Asterisk Developers.


Bugs: ASTERISK-24841
https://issues.asterisk.org/jira/browse/ASTERISK-24841


Repository: Asterisk


Description
---

With this patch, chan_pjsip/res_pjsip now sets the native formats to the
codecs negotiated by a call.

* The changes in chan_pjsip.c and res_pjsip_sdp_rtp.c set the native
formats to include all the negotiated audio codecs instead of only the
initial preferred audio codec and later the currently received audio
codec.

* The audio frame handling in channel.c:ast_read() is more streamlined and
will automatically adjust to changes in received frame formats.  The new
policy is to remove translation and pass the new frame format to the
receiver except if the translation was to a signed linear format.  A more
long winded version is commented in ast_read() along with some caveats.

* The audio frame handling in channel.c:ast_write() is more streamlined
and will automatically adjust any needed translation to changes in the
frame formats sent.  Frame formats sent can change for many reasons such
as a recording is being played back or the bridged peer changed the format
it sends.  Since it is a normal expectation that sent formats can change,
the codec mismatch warning message is demoted to a debug message.

* Removed the short circuit check in
channel.c:ast_channel_make_compatible_helper().  Two party bridges need to
make channels compatible with each other.  However, transfers and moving
channels among bridges can result in otherwise compatible channels having
sub-optimal translation paths if the make compatible check is short
circuited.  A result of forcing the reevaluation of channel compatibility
is that the asterisk.conf:transcode_via_slin and codecs.conf:genericplc
options take effect consistently now.  It is unfortunate that these two
options are enabled by default and negate some of the benefits to the
changes in channel.c:ast_read() by forcing translation through signed
linear on a two party bridge.

* Improved the softmix bridge technology to better control the translation
of frames to the bridge.  All of the incoming translation is now normally
handled by ast_read() instead of splitting any translation steps between
ast_read() and the slin factory.  If any frame comes in with an unexpected
format then the translation path in ast_read() is updated for the next
frame and the slin factory handles the current frame translation.

This is the final patch in a series of patches aimed at improving
translation path choices.  The other patches are on the following reviews:
https://reviewboard.asterisk.org/r/4600/
https://reviewboard.asterisk.org/r/4605/


Diffs
-

  /branches/13/res/res_pjsip_sdp_rtp.c 434526 
  /branches/13/main/channel.c 434526 
  /branches/13/include/asterisk/channel.h 434526 
  /branches/13/channels/chan_pjsip.c 434526 
  /branches/13/bridges/bridge_softmix.c 434526 

Diff: https://reviewboard.asterisk.org/r/4609/diff/


Testing
---

* The testsuite still passes as well as it ever has.

* Manual SIP and DTMF attended transfers still function.  With all patches
in the series applied, if a low speed party transfers a higher speed party
to another high speed party then when the transfer completes the resulting
call works at the higher speed.  Without the patch the resulting call may
go through a sub-optimal translation path with reduced audio quality.

* ConfBridge bridges are able to change mixing rates as different speed
participants enter and leave the bridge.  Sound files played back to
individual participants may go out with a different codec than the
participant sends to the conference.  If the conference bridge is mixing
at a lower rate than a participant then the conference media may go out
with a different codec than the participant sends to the conference.

* Used app_originate to setup a call through a non-optimizing local
channel.  The resulting call used the same codecs as before the patch even
between parties with different speeds.


Thanks,

rmudgett

-- 
_
-- 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] 4608: res_pjsip_phoneprov_provider: Fix reference leak on unload

[George Joseph]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4608/
---

Review request for Asterisk Developers and Corey Farrell.


Bugs: ASTERISK-24935
https://issues.asterisk.org/jira/browse/ASTERISK-24935


Repository: Asterisk


Description
---

res_pjsip_phoneprov_provider was leaking references to phoneprov objects due to 
a missing OBJ_NODATA in an ao2_callback in load_users().  Rather than adding 
the OBJ_NODATA, I changed load_users to use a more straightforward 
ao2_iterator.  This plugged the leak but exposed an unload order issue between 
res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip.

res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then res_pjsip. 
 Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery instance, when it 
unloads, it's objects are still in the sorcery instance.  When res_pjsip 
unloads, it destroys all its objects including res_pjsip_phoneprov_provider's.  
The phoneprov destructor then attempts to unregister the extension from 
res_phoneprov but because res_phoneprov is already cleaned up, its users 
container is gone and we get a FRACK.

Simple solution, check for the NULL users container before attempting to remove 
the entry. Duh.


Diffs
-

  branches/13/res/res_pjsip_phoneprov_provider.c 434447 
  branches/13/res/res_phoneprov.c 434447 

Diff: https://reviewboard.asterisk.org/r/4608/diff/


Testing
---

Ran tests/res_phoneprov/res_phoneprov_provider.  No leaks in 
res_pjsip_phoneprov_provider and no FRACKs.


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

[asterisk-dev] Change in testsuite[master]: rest_api/applications/stasisstatus: Make run-test executable

[Mark Michelson (Code Review)]

Mark Michelson has posted comments on this change.

Change subject: rest_api/applications/stasisstatus: Make run-test executable
..


Patch Set 1: Code-Review+2 Verified+1

-- 
To view, visit https://gerrit.asterisk.org/36
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan 
Gerrit-Reviewer: Ashley Sanders 
Gerrit-Reviewer: Mark Michelson 
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]: rest_api/applications/stasisstatus: Make run-test executable

[Ashley Sanders (Code Review)]

Ashley Sanders has posted comments on this change.

Change subject: rest_api/applications/stasisstatus: Make run-test executable
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.asterisk.org/36
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan 
Gerrit-Reviewer: Ashley Sanders 
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] 4605: translate.c: Only select audio codecs to determine the best translation choice.

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4605/
---

(Updated April 9, 2015, 11:50 a.m.)


Review request for Asterisk Developers.


Bugs: ASTERISK-21777, ASTERISK-24380 and ASTERISK-24841
https://issues.asterisk.org/jira/browse/ASTERISK-21777
https://issues.asterisk.org/jira/browse/ASTERISK-24380
https://issues.asterisk.org/jira/browse/ASTERISK-24841


Repository: Asterisk


Description (updated)
---

Given a source capability of h264 and ulaw, a destination capability of
h264 and g722 then ast_translator_best_choice() would pick h264 as the
best choice even though h264 is a video codec and Asterisk only supports
translation of audio codecs.  When the audio starts flowing, there are
warnings about a codec mismatch when the channel tries to write a frame to
the peer.

* Made ast_translator_best_choice() only select audio codecs.

* Restore a check in channel.c:set_format() lost after v1.8 to prevent
trying to set a non-audio codec.

This is an intermediate patch for a series of patches aimed at improving
translation path choices for ASTERISK-24841.

This patch is a partial fix for ASTERISK-21777 as the v11 version of
ast_translator_best_choice() does the same thing.  However, chan_sip
somehow sets the nativeformats capabilities to just h264 or empty.


This patch will be backported to v11 as a partial fix for ASTERISK-21777.


Diffs
-

  /branches/13/main/translate.c 434447 
  /branches/13/main/channel.c 434447 

Diff: https://reviewboard.asterisk.org/r/4605/diff/


Testing
---

Modified chan_pjsip.c:chan_pjsip_new() to force inclusion of the h264
video codec in the native capabilities.  Without the patch I get the path
translation warnings.  With the patch I don't get the warnings.


Thanks,

rmudgett

-- 
_
-- 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] 4608: res_pjsip_phoneprov_provider: Fix reference leak on unload

[Corey Farrell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4608/#review15167
---

Ship it!


Ship It!

- Corey Farrell


On April 9, 2015, 3:57 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4608/
> ---
> 
> (Updated April 9, 2015, 3:57 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Bugs: ASTERISK-24935
> https://issues.asterisk.org/jira/browse/ASTERISK-24935
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> res_pjsip_phoneprov_provider was leaking references to phoneprov objects due 
> to a missing OBJ_NODATA in an ao2_callback in load_users().  Rather than 
> adding the OBJ_NODATA, I changed load_users to use a more straightforward 
> ao2_iterator.  This plugged the leak but exposed an unload order issue 
> between res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip.
> 
> res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then 
> res_pjsip.  Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery 
> instance, when it unloads, it's objects are still in the sorcery instance.  
> When res_pjsip unloads, it destroys all its objects including 
> res_pjsip_phoneprov_provider's.  The phoneprov destructor then attempts to 
> unregister the extension from res_phoneprov but because res_phoneprov is 
> already cleaned up, its users container is gone and we get a FRACK.
> 
> Simple solution, check for the NULL users container before attempting to 
> remove the entry. Duh.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_phoneprov_provider.c 434447 
>   branches/13/res/res_phoneprov.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4608/diff/
> 
> 
> Testing
> ---
> 
> Ran tests/res_phoneprov/res_phoneprov_provider.  No leaks in 
> res_pjsip_phoneprov_provider and no FRACKs.
> 
> 
> 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

[asterisk-dev] [Code Review] 4607: bridge_softmix.c, channel.c: Minor code simplification and cleanup.

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4607/
---

Review request for Asterisk Developers.


Repository: Asterisk


Description
---

* Made code easier to follow in bridge_softmix.c:analyse_softmix_stats()
and made some debug messages more helpful.

* Made some debug and warning messages more helpful in
channel.c:set_format().


Diffs
-

  /branches/13/main/channel.c 434509 
  /branches/13/bridges/bridge_softmix.c 434509 

Diff: https://reviewboard.asterisk.org/r/4607/diff/


Testing
---

Items found while researching code for ASTERISK-24841.  It compiles.  Warm 
fuzzy because it was part of code put in while testing overall code for 
ASTERISK-24841.


Thanks,

rmudgett

-- 
_
-- 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] 4606: chan_sip: make progressinband default to no

[Kevin Harwell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4606/
---

(Updated April 9, 2015, 12:05 p.m.)


Review request for Asterisk Developers.


Changes
---

Addressed review feedback.


Bugs: ASTERISK-24835
https://issues.asterisk.org/jira/browse/ASTERISK-24835


Repository: Asterisk


Description
---

After the "progressinband" value setting of "never" was updated to never send a 
183 this separated its use from the "no" value. Since "never" was the default, 
but most users probably expect "no" this patch updates the default for the 
"progressinband" setting to "no".


Diffs (updated)
-

  branches/13/configs/samples/sip.conf.sample 434508 
  branches/13/channels/sip/include/sip.h 434508 
  branches/13/channels/chan_sip.c 434508 
  branches/13/CHANGES 434508 

Diff: https://reviewboard.asterisk.org/r/4606/diff/


Testing
---

Started Asterisk and loaded chan_sip with the new default value for 
progressinband. Check to make sure that is what it was set to. Changed the 
setting to the other values and reloaded/checked between each to make sure 
those got set correctly as well.


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] 4603: pjsip: Add external PJSIP resolver implementation using core DNS API

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4603/#review15162
---


This review can be summed up in 2 words: "NAPTR ARGH"


/trunk/include/asterisk/dns_query_set.h


Seeing the implementation, I'd add some sort of note to this doxygen that 
states that the returned query is not safe to use after you decrement your 
reference to query_set.



/trunk/main/dns_query_set.c


I don't think it would hurt to increase this to 4 or 5, tbh.



/trunk/main/dns_query_set.c


A state variable should be added so that if someone attempts to add to the 
query set after they have called ast_dns_query_set_resolve_async(), this 
immediately returns an error.

By doing this, you can have a guarantee that the AST_VECTOR of queries on 
the query set cannot change size once the resolution has begun. This is 
important because otherwise, when dns_query_set_callback() is called, it may 
never call the query_set's callback method since the vector has grown larger 
than the number of in-flight queries.



/trunk/main/dns_recurring.c


I don't understand why these #includes were added. Same goes for dns_srv.c 
and dns_naptr.c



/trunk/res/res_pjsip/pjsip_resolver.c


It's probably okay for it to be this way, but because of the way that PJSIP 
defines its transport constants, this array is larger than one might expect it 
to be. The indices of this array are 1, 2, 3, 129, 130, and 131.

If it's not too troublesome, you may want to make this array more compact 
and have a function that retrieves the appropriate index in the array given a 
PJSIP transport type.



/trunk/res/res_pjsip/pjsip_resolver.c


Why + 10?



/trunk/res/res_pjsip/pjsip_resolver.c


NAPTR has a few restrictions that SRV does not have. With SRV, you sort the 
records and you can continue to fail over to the next one in the list, no 
matter the priority or weight of each individual record.

With NAPTR, RFC 3403 section 4.1 places the following restriction:

"The important difference between Order and Preference is that once a match 
is found the client MUST NOT consider records with a different Order but they 
MAY process records with the same Order but different Preferences"

This means you need to examine the Order field of the records more closely.

Defining "match" is a bit tricky but I'm going to use the NAPTR example in 
RFC 3264 Section 4.1 as a reference. In that example, it shows three NAPTR 
records for the three different SIP services provided, each with a different 
order. It then states:

"This indicates that the server supports TLS over TCP, TCP, and UDP, in 
that order of preference.  Since the client supports TCP and UDP, TCP will be 
used, targeted to a host determined by an SRV lookup of _sip._tcp.example.com."

By my reading, a "match" means that you recognize the service in the NAPTR 
record as being SIP-related AND that you support the transport protocol that 
the record pertains to.

In the case of this loop, then, if you find a NAPTR record with a SIP+D2X 
service that you recognize and whose transport you support, you should not do 
anything with NAPTR records that have a higher order value than this first 
match. You can process records with the same order but higher preference 
number, though. That's kosher.



/trunk/res/res_pjsip/pjsip_resolver.c


There is a lot of repeated code in these if statements. The variants are

PJSIP_TRANSPORT_X transport
SIP+D2X NAPTR flag

This could be factored into a function of its own.



/trunk/res/res_pjsip/pjsip_resolver.c


For all of these, you may want to double-check that 
ast_dns_naptr_get_replacement() is a non-zero length string.



/trunk/res/res_pjsip/pjsip_resolver.c


RFC 3263 says:

"These NAPTR records provide a mapping from a domain to the SRV record for 
contacting a server with the specific transport protocol in the NAPTR services 
field. The resource record will contain an empty regular expression and a 
replacement value, which is the SRV record for that particular transport 
protocol."

Unfortunately, this is the only language in the RFC that details what the 
expected content of a SIP+D2X service record will be. In the test plan I'm 

Re: [asterisk-dev] [Code Review] 4606: chan_sip: make progressinband default to no

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4606/#review15159
---



branches/13/CHANGES


Expand this to clarify that the actual behavior will match that of previous 
versions.


- Joshua Colp


On April 9, 2015, 4:35 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4606/
> ---
> 
> (Updated April 9, 2015, 4:35 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24835
> https://issues.asterisk.org/jira/browse/ASTERISK-24835
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> After the "progressinband" value setting of "never" was updated to never send 
> a 183 this separated its use from the "no" value. Since "never" was the 
> default, but most users probably expect "no" this patch updates the default 
> for the "progressinband" setting to "no".
> 
> 
> Diffs
> -
> 
>   branches/13/configs/samples/sip.conf.sample 434508 
>   branches/13/channels/sip/include/sip.h 434508 
>   branches/13/channels/chan_sip.c 434508 
>   branches/13/CHANGES 434508 
> 
> Diff: https://reviewboard.asterisk.org/r/4606/diff/
> 
> 
> Testing
> ---
> 
> Started Asterisk and loaded chan_sip with the new default value for 
> progressinband. Check to make sure that is what it was set to. Changed the 
> setting to the other values and reloaded/checked between each to make sure 
> those got set correctly as well.
> 
> 
> 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]: rest_api/applications/stasisstatus: Make run-test executable

[Mark Michelson (Code Review)]

Mark Michelson has submitted this change and it was merged.

Change subject: rest_api/applications/stasisstatus: Make run-test executable
..


rest_api/applications/stasisstatus: Make run-test executable

If it isn't executable, it can't run!

Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
---
M tests/rest_api/applications/stasisstatus/run-test
1 file changed, 0 insertions(+), 0 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, approved; Verified
  Ashley Sanders: Looks good to me, but someone else must approve



diff --git a/tests/rest_api/applications/stasisstatus/run-test 
b/tests/rest_api/applications/stasisstatus/run-test
old mode 100644
new mode 100755

-- 
To view, visit https://gerrit.asterisk.org/36
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibf0647d37c9bba0318f251e1040c03372bce1b54
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan 
Gerrit-Reviewer: Ashley Sanders 
Gerrit-Reviewer: Mark Michelson 

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4608: res_pjsip_phoneprov_provider: Fix reference leak on unload

[Corey Farrell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4608/#review15164
---


I think I still prefer ao2_callback over ao2_iterator for taking an action 
against every object in a container, but don't feel strongly enough to hold 
this up.  Minor nit with the return value, then we can ship this.


branches/13/res/res_pjsip_phoneprov_provider.c


If we're sticking with the ao2_iterator lets loose the 'int' return, just 
make this function void.



branches/13/res/res_pjsip_phoneprov_provider.c


Remove..



branches/13/res/res_pjsip_phoneprov_provider.c


To record our brief discussion about this change:
 gtjoseph: so one thing I don't see how using an ao2_iterator 
instead of ao2_callback in users_apply_handler makes it more straight forward..
 I don't want to hold up the fix, but that change doesn't 
make much sense to me
 there's no worry about the callback return value, OBJ_NODATA, 
etc.  Get the container and iterate over it.   It also eliminates a lot of 
overhead in the ao2_traversal code.
 unless the callback is controlling the traversal with it's 
return code, there's no point.


- Corey Farrell


On April 9, 2015, 3:20 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4608/
> ---
> 
> (Updated April 9, 2015, 3:20 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Bugs: ASTERISK-24935
> https://issues.asterisk.org/jira/browse/ASTERISK-24935
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> res_pjsip_phoneprov_provider was leaking references to phoneprov objects due 
> to a missing OBJ_NODATA in an ao2_callback in load_users().  Rather than 
> adding the OBJ_NODATA, I changed load_users to use a more straightforward 
> ao2_iterator.  This plugged the leak but exposed an unload order issue 
> between res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip.
> 
> res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then 
> res_pjsip.  Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery 
> instance, when it unloads, it's objects are still in the sorcery instance.  
> When res_pjsip unloads, it destroys all its objects including 
> res_pjsip_phoneprov_provider's.  The phoneprov destructor then attempts to 
> unregister the extension from res_phoneprov but because res_phoneprov is 
> already cleaned up, its users container is gone and we get a FRACK.
> 
> Simple solution, check for the NULL users container before attempting to 
> remove the entry. Duh.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_phoneprov_provider.c 434447 
>   branches/13/res/res_phoneprov.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4608/diff/
> 
> 
> Testing
> ---
> 
> Ran tests/res_phoneprov/res_phoneprov_provider.  No leaks in 
> res_pjsip_phoneprov_provider and no FRACKs.
> 
> 
> 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

[asterisk-dev] Change in testsuite[master]: Testsuite: Caller & callee initiated local attended transfer...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: Testsuite: Caller & callee initiated local attended transfers 
to application.
..


Patch Set 1:

(7 comments)

https://gerrit.asterisk.org/#/c/34/1//COMMIT_MSG
Commit Message:

Line 11: 
Please add the ASTERISK issue to the commit message.


https://gerrit.asterisk.org/#/c/34/1/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/configs/ast1/pjsip.conf
File 
tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/configs/ast1/pjsip.conf:

Line 25: ; TODO remove
   : ;contact=sip:alice@127.0.0.1:5061\;transport=udp
I'd just remove it at this point :-)


Line 40: ; TODO remove
   : ;contact=sip:bob@127.0.0.1:5062\;transport=udp
Remove this as well.


https://gerrit.asterisk.org/#/c/34/1/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/test-config.yaml
File 
tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/test-config.yaml:

Line 1
Since 12 isn't receiving a whole lot of attention - and it is plausible that 
*something* would go wrong in attended transfers in 12.0.0 - I'd make the 
minversion here 13.0.0.


https://gerrit.asterisk.org/#/c/34/1/tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/configs/ast1/pjsip.conf
File 
tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/configs/ast1/pjsip.conf:

Line 25: ; TODO remove
   : ;contact=sip:alice@127.0.0.1:5061\;transport=udp
Remove


Line 40: ; TODO remove
   : ;contact=sip:bob@127.0.0.1:5062\;transport=udp
Remove.


https://gerrit.asterisk.org/#/c/34/1/tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/test-config.yaml
File 
tests/channels/pjsip/transfers/attended_transfer/nominal/caller_local_app/test-config.yaml:

Line 174: minversion: '12.0.0'
minversion of 13.0.0


-- 
To view, visit https://gerrit.asterisk.org/34
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I61b70f83013b8d37de3fe1eea53c8dd2d026d2e0
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: John Bigelow 
Gerrit-Reviewer: Matt Jordan 
Gerrit-HasComments: Yes

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

[asterisk-dev] Change in testsuite[master]: pjsip: Add basic resolver tests covering A/AAAA, SRV, and NA...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: pjsip: Add basic resolver tests covering A/, SRV, and NAPTR.
..


Patch Set 1:

(1 comment)

https://gerrit.asterisk.org/#/c/31/1/tests/channels/pjsip/resolver/a/sipp/uas.xml
File tests/channels/pjsip/resolver/a/sipp/uas.xml:

Line 4: 
Copy/paste: this scenario doesn't receive an INVITE with video :-)

This comment will apply to all of the UAS scenarios in this patchset.


-- 
To view, visit https://gerrit.asterisk.org/31
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8690d6b2441937ab9d7fea6f1e41c3d6985a1d9e
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp 
Gerrit-Reviewer: Jared K. Smith 
Gerrit-Reviewer: Matt Jordan 
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] 4604: loader/main: Don't set ast_fully_booted until deferred reloads are processed

[Corey Farrell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4604/#review15154
---

Ship it!


Ship It!

- Corey Farrell


On April 9, 2015, 10:57 a.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4604/
> ---
> 
> (Updated April 9, 2015, 10:57 a.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Until we have a true module management facility it's sometimes necessary for 
> one module to force a reload on another before its own load is complete.  If 
> Asterisk isn't fully booted yet, these reloads are deferred.  The problem is 
> that asterisk reports fully booted before processing the deferred reloads 
> which means Asterisk really isn't quite ready when it says it is.
> 
> This patch moves the report of fully booted after the processing of the 
> deferred reloads is complete.
> 
> 
> Diffs
> -
> 
>   branches/13/main/loader.c 434447 
>   branches/13/main/asterisk.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4604/diff/
> 
> 
> Testing
> ---
> 
> Since the pjsip stack has the most number of related modules, I ran the 
> channels/pjsip testsuite to make sure there aren't any issues.  All tests 
> passed.
> 
> 
> 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

[Y Ateya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4588/
---

(Updated April 9, 2015, 2:21 p.m.)


Review request for Asterisk Developers.


Changes
---

- Removed useless log message
- Used sscanf instead of atoi


Bugs: ASTERISK-24939
https://issues.asterisk.org/jira/browse/ASTERISK-24939


Repository: Asterisk


Description
---

The section 4.1 in call token changes to IAX protocol 
(http://downloads.asterisk.org/pub/security/IAX2-security.html):

 "The token timeout will be hard coded at 10 seconds for now. However, it may 
be made configurable at some point if it seems to be a useful addition"

In case of lagged network cases (or bad network which required multiple 
retries) 10 seconds is not enough.

Changes:
 - Change name of MAX_CALLTOKEN_DELAY to lower case and remove const.
 - Add general configuration variable `calltokenexpiration`


Diffs (updated)
-

  trunk/configs/samples/iax.conf.sample 432806 
  trunk/channels/chan_iax2.c 432806 

Diff: https://reviewboard.asterisk.org/r/4588/diff/


Testing
---


Thanks,

Y Ateya

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4585: res_pjsip: Refactor endpt_send_request to include transaction timeout

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4585/#review15149
---


I've also sent an email to Teluu for their thoughts on this. Specifically 
whether the timeout parameter is deprecated (or whether it should work but does 
not), and any suggestions on how we can accomplish this.


branches/13/res/res_pjsip.c


There's a race condition here. The ast_sched_find_data may return data but 
when AST_SCHED_DEL is invoked the scheduled item could wrong. Therefore 
ast_sched_find_data isn't really needed.

As well it's possible for the scheduled item to be running.



branches/13/res/res_pjsip.c


So!

If both the transaction timer and the timer here execute at the same time 
it's possible for the transaction_timeout_cb to operate on a transaction that 
may be destroyed.

The transaction layer uses group locks to overcome this, which is only 
available in recent PJSIP releases.

I think instead of using our own scheduler the PJSIP one should be used 
instead. There's a guarantee that only one scheduled item will occur at a time, 
so you won't clash with the transaction timers.


- Joshua Colp


On April 3, 2015, 8:12 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4585/
> ---
> 
> (Updated April 3, 2015, 8:12 p.m.)
> 
> 
> 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] 4597: res_pjsip: add CLI command to show global and system configuration

[rnewton]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4597/#review15148
---

Ship it!


Ran the command, format looks good - all the settings show up and reflect 
accurately. Changed settings, I see the new settings after restart.

- rnewton


On April 8, 2015, 5:52 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4597/
> ---
> 
> (Updated April 8, 2015, 5:52 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24918
> https://issues.asterisk.org/jira/browse/ASTERISK-24918
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Added a new CLI command for res_pjsip that shows both global and system 
> configuration settings:
> 
> pjsip show settings
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 434423 
>   branches/13/res/res_pjsip/config_system.c 434423 
>   branches/13/res/res_pjsip/config_global.c 434423 
>   branches/13/res/res_pjsip.c 434423 
>   branches/13/CHANGES 434423 
> 
> Diff: https://reviewboard.asterisk.org/r/4597/diff/
> 
> 
> Testing
> ---
> 
> Ran the command and checked output. Changed some options and reloaded and 
> made sure global settings changed, but system ones did not. Changed some 
> settings again and restarted and made sure both global and system changes 
> took effect. Also removed the sections completely from the pjsip.conf file 
> and made sure the defaults were shown.
> 
> 
> 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] 4533: clang compiler warning: -Wtautological-compare

[Diederik de Groot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---

(Updated April 9, 2015, 7:47 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 434469


Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917


Repository: Asterisk


Description
---

clang's static analyzer will throw quite a number warnings / errors during 
compilation, some of which can be very helpfull in finding corner-case bugs

clang compiler warning:-Wtautological-compare

Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value < 0

/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is 
problematic. Extending the enum in curl/curl.h is not possible either. I opted 
to use the enum last entry (CURL_LAST) which is currently not used for any 
thing. Another option would be to use one of the OBSOLETE VALUES like 16. 
Neither way is very nice though.

/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by 
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse

/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be prefered 
for such an enum value.

/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.

/branches/13/main/enum.c:260
dn_expand return signed int

/branches/13/main/event.c:202
enum type cannot be < 0

/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if < 0. Not 
sure what should happend when freq1 / freq2 are 0 already... (needs recheck by 
source owner)

/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State

/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be <0

/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if < 0 does not make sence. 
Not 100% if encode_length has side effects, so left the actual call the this 
function in place. (Needs to be rechecked by code-owner)

/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from 
ast_hint_presence_state.. Not very nice, but did not want to change the 
signature of this function.

/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be < 0

/branches/13/res/res_stasis_recording.c:598/604
recording->state / operation is enum and cannot be < 0

/branches/13/res/res_security_log.c:992
enum event_type cannot be <0

use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the 
lower limit which is always true.


Diffs
-

  /branches/13/res/res_stasis_recording.c 433444 
  /branches/13/res/res_stasis_playback.c 433444 
  /branches/13/res/res_pjsip_exten_state.c 433444 
  /branches/13/res/ari/resource_channels.c 433444 
  /branches/13/res/ari/resource_bridges.c 433444 
  /branches/13/main/udptl.c 433444 
  /branches/13/main/security_events.c 433444 
  /branches/13/main/presencestate.c 433444 
  /branches/13/main/indications.c 433444 
  /branches/13/main/event.c 433444 
  /branches/13/main/enum.c 433444 
  /branches/13/main/cel.c 433444 
  /branches/13/main/app.c 433444 
  /branches/13/include/asterisk/utils.h 433444 
  /branches/13/include/asterisk/logger.h 433444 
  /branches/13/include/asterisk/cel.h 433444 
  /branches/13/include/asterisk/app.h 433444 
  /branches/13/funcs/func_curl.c 433444 
  /branches/13/channels/pjsip/dialplan_functions.c 433444 
  /branches/13/channels/chan_skinny.c 433444 

Diff: https://reviewboard.asterisk.org/r/4533/diff/


Testing
---

Compiles without warning


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]: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.

[Corey Farrell (Code Review)]

Hello Anonymous Coward #119,

I'd like you to reexamine a change.  Please visit

https://gerrit.asterisk.org/35

to look at the new patch set (#3).

Change subject: res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.
..

res_phoneprov_pjsip: Disconnect HTTP before stopping Asterisk.

Leaving an HTTP connection open causes Asterisk to report a reference
leak.  This change ensures the connection is closed before stopping
Asterisk.

Change-Id: I5553af9cbfdc1d68110036eaadcfe5db3570928f
---
M tests/phoneprov/res_phoneprov_pjsip/run-test
1 file changed, 6 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/testsuite refs/changes/35/35/3
-- 
To view, visit https://gerrit.asterisk.org/35
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5553af9cbfdc1d68110036eaadcfe5db3570928f
Gerrit-PatchSet: 3
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell 
Gerrit-Reviewer: Anonymous Coward #119
Gerrit-Reviewer: Matt Jordan 

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

[asterisk-dev] Change in testsuite[master]: pjsip: Add test for OPTIONS requests received in-dialog.

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: pjsip: Add test for OPTIONS requests received in-dialog.
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.asterisk.org/37
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I137c66e4faad5212040f6aded6be2aac20fc473d
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp 
Gerrit-Reviewer: Matt Jordan 
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] 4547: clang compiler warning: braces-around-scalar-initializer

[Diederik de Groot]

> On March 28, 2015, 3:59 p.m., Matt Jordan wrote:
> > /branches/13/channels/chan_iax2.c, lines 7674-7676
> > 
> >
> > I really dislike this warning.
> > 
> > In C, it has always been perfectly valid to initialize all members of a 
> > struct using { 0 } as a universal zero-initializer. This nomenclature 
> > actually makes it less readable, as it makes it look like we only wanted to 
> > initialize the .frametype member, and ignored the rest. That may not be the 
> > actual effect, but the syntax here is not clearer by any stretch.
> > 
> > :-(
> > 
> > Interestingly, when compiling with clang 3.4, I don't get this warning 
> > issued. What version are you compiling with? And what do you think about 
> > simply ignoring this warning?
> 
> Diederik de Groot wrote:
> Using "struct ast_frame f = { 0, };" is perfectly fine and does not raise 
> an error
> 
> However: "struct ast_frame f = { AST_FRAME_CONTROL, { 
> AST_CONTROL_CONGESTION } };" however does raise this warning, which does make 
> a little more sense. 
> 
> I updated all of these (i hope i got them all) so that they all use the 
> same syntax. In case someone ones to update/change/extend one of them.
> 
> FYI: There is one major benefit in using the named variety, namely 
> allowing you to change the order of the fields in the struct without 
> consequence. For example the ast_frame struct could be optimized a little by 
> reordering the fields to improve packing, as in: 
> 
> struct ast_frame {
> /*! Kind of frame */
> enum ast_frame_type frametype;
> /*! Length of data */
> int datalen;
> /*! Subclass, frame dependent */
> struct ast_frame_subclass subclass;
> /*! Number of samples in this frame */
> int samples;
> /*! Was the data malloc'd?  i.e. should we free it when we 
> discard the frame? */
> int mallocd;
> /*! The number of bytes allocated for a malloc'd frame header */
> size_t mallocd_hdr_len;
> /*! How many bytes exist _before_ "data" that can be used if 
> needed */
> int offset;
> /*! Misc. frame flags */
> unsigned int flags;
> /*! Optional source of frame for debugging */
> const char *src;
> /*! Pointer to actual data */
> union { void *ptr; uint32_t uint32; char pad[8]; } data;
> /*! Global delivery time */
> struct timeval delivery;
> /*! For placing in a linked list */
> AST_LIST_ENTRY(ast_frame) frame_list;
> /*! Timestamp in milliseconds */
> long ts;
> /*! Length in milliseconds */
> long len;
> /*! Sequence number */
> int seqno;
> };
> 
> would get rid of some of the padding (on x86_64).
> 
> Note: this warning might have occured after playing with the struct 
> layout of ast_frame to be honest (Need to recheck this). If so this change 
> should be considered an enhancement.
> Note2: I compile using clang-3.6

Any update on this ?


- Diederik


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4547/#review14941
---


On March 27, 2015, 7:34 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4547/
> ---
> 
> (Updated March 27, 2015, 7:34 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:braces-around-scalar-initializer
> 
> 
> Diffs
> -
> 
>   /branches/13/res/res_pjsip_dtmf_info.c 433444 
>   /branches/13/channels/sig_ss7.c 433444 
>   /branches/13/channels/sig_pri.c 433444 
>   /branches/13/channels/pjsip/dialplan_functions.c 433444 
>   /branches/13/channels/console_gui.c 433444 
>   /branches/13/channels/chan_unistim.c 433444 
>   /branches/13/channels/chan_skinny.c 433444 
>   /branches/13/channels/chan_sip.c 433444 
>   /branches/13/channels/chan_oss.c 433444 
>   /branches/13/channels/chan_mgcp.c 433444 
>   /branches/13/channels/chan_iax2.c 433444 
>   /branches/13/channels/chan_dahdi.c 433444 
>   /branches/13/channels/chan_console.c 433444 
>   /branches/13/channels/chan_alsa.c 433444 
>   /bran

Re: [asterisk-dev] progressinband in chan_sip default value

[Steve Davies]

Hi,

I submitted that change, as we have need for not the 'no' and the 'never'
cases on different devices/trunks etc, and before the patch they were
almost the same.

I completely agree with the above suggestion from Kevin. I always set
"progressinband" manually for all of my device definitions, so had never
realised that the patch was changing the normal/default behaviour.

Regards,
Steve

On Wed, 8 Apr 2015 at 18:00 Kevin Harwell  wrote:

> Greetings,
>
> A few months ago an issue [1] was reported that when the "progressinband"
> option found in chan_sip was set to "never" it was not working correctly in
> some scenarios. A fix for the issue was committed [2]. However, the fix had
> a side effect of changing the scope of "never" somewhat and since "never"
> is the default this can potentially cause some confusion or unexpected
> behavior when upgrading.
>
> It's believed that changing the default from "never" to "no" would be more
> appropriate for most users as then "progressinband" would default to
> working similar to how it was before.
>
> Thoughts on this? Leave it defaulting to "never"? Changing it to "no"
> would be fine?
>
> [1] https://issues.asterisk.org/jira/browse/ASTERISK-23972
> [2] https://reviewboard.asterisk.org/r/3700
>
> --
>
> Kevin Harwell
> Digium, Inc. | Software Developer
> 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
> Check us out at: http://digium.com & http://asterisk.org
>
>  --
> _
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
>
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>http://lists.digium.com/mailman/listinfo/asterisk-dev
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block

[Diederik de Groot]

> On March 29, 2015, 5:01 a.m., Diederik de Groot wrote:
> > /branches/13/configure.ac, line 389
> > 
> >
> > move the raii compiler checks to their own m4 file and called the 
> > checking function a little earlier in the configure process

Is this ok, or would you rather keep everything together in configure.ac. I 
think splitting out some stuff makes it a little more easy to maintain / test


- Diederik


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4543/#review14954
---


On March 29, 2015, 5:15 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4543/
> ---
> 
> (Updated March 29, 2015, 5:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
> https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> RAII-clang reentrancy / Updating variable from inside a _block
> 
> To update the varname variable from inside the cleanup block it needs to be 
> decorated with "__block" as in:
> __block vartype varname = ctor;
> 
> 
> Diffs
> -
> 
>   /branches/13/include/asterisk/utils.h 433444 
>   /branches/13/contrib/scan-build PRE-CREATION 
>   /branches/13/configure.ac 433444 
>   /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4543/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4499: Support in dialog OPTIONS

[Joshua Colp]

> On March 23, 2015, 8:01 p.m., Matt Jordan wrote:
> > Thanks for the patch! I've clicked the Ship It button, although the same 
> > statement about requiring tests for things going into Asterisk 13 that I 
> > made on the DTMF review applies here as well.
> > 
> > In this particular case, a test for this patch should be done using SIPp, 
> > as it is pretty easy to construct an inbound INVITE request and put an 
> > OPTION request in-dialog with that INVITE request.
> > 
> > Most of the tests in channels/pjsip use SIPp to drive the tests, and so 
> > there is a lot of material to base a test on. We also have sample SIPp 
> > scenarios to use as a template in the contrib/sipp folder.
> > 
> > If you have any questions about where to start with that, please don't 
> > hesitate to ask on the asterisk-dev mailing list/#asterisk-dev.

A testsuite test has now been published for review at 
https://gerrit.asterisk.org/#/c/37/


- Joshua


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4499/#review14774
---


On March 18, 2015, 9:01 a.m., yaron nahum wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4499/
> ---
> 
> (Updated March 18, 2015, 9:01 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24862
> https://issues.asterisk.org/jira/browse/ASTERISK-24862
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Respond to OPTIONS message sent on an existing dialog with 200OK. 
> This feature is vital in order to interoperate with some switches that send 
> OPTIONS message periodically per active call to make sure it is still alive. 
> Not responding would cause the switch to disconnect the call. 
> This functionality used to work on the old SIP channel, but was not 
> implemented on PJSIP.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_pjsip_dlg_options.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4499/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> yaron nahum
> 
>

-- 
_
-- 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] 4604: loader/main: Don't set ast_fully_booted until deferred reloads are processed

[George Joseph]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4604/
---

(Updated April 9, 2015, 8:57 a.m.)


Review request for Asterisk Developers and Corey Farrell.


Changes
---

Addressed Matt's nitpicks and updated test results.


Repository: Asterisk


Description
---

Until we have a true module management facility it's sometimes necessary for 
one module to force a reload on another before its own load is complete.  If 
Asterisk isn't fully booted yet, these reloads are deferred.  The problem is 
that asterisk reports fully booted before processing the deferred reloads which 
means Asterisk really isn't quite ready when it says it is.

This patch moves the report of fully booted after the processing of the 
deferred reloads is complete.


Diffs (updated)
-

  branches/13/main/loader.c 434447 
  branches/13/main/asterisk.c 434447 

Diff: https://reviewboard.asterisk.org/r/4604/diff/


Testing (updated)
---

Since the pjsip stack has the most number of related modules, I ran the 
channels/pjsip testsuite to make sure there aren't any issues.  All tests 
passed.


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] 4608: res_pjsip_phoneprov_provider: Fix reference leak on unload

[George Joseph]

> On April 9, 2015, 1:49 p.m., Corey Farrell wrote:
> > branches/13/res/res_pjsip_phoneprov_provider.c, lines 358-363
> > 
> >
> > To record our brief discussion about this change:
> >  gtjoseph: so one thing I don't see how using an 
> > ao2_iterator instead of ao2_callback in users_apply_handler makes it more 
> > straight forward..
> >  I don't want to hold up the fix, but that change doesn't 
> > make much sense to me
> >  there's no worry about the callback return value, 
> > OBJ_NODATA, etc.  Get the container and iterate over it.   It also 
> > eliminates a lot of overhead in the ao2_traversal code.
> >  unless the callback is controlling the traversal with it's 
> > return code, there's no point.

Actually, I think more people should use an ao2_iterator instead of 
ao2_callback when they can.  That internal_ao2_traverse code is ugly. :)


- George


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4608/#review15164
---


On April 9, 2015, 1:57 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4608/
> ---
> 
> (Updated April 9, 2015, 1:57 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Bugs: ASTERISK-24935
> https://issues.asterisk.org/jira/browse/ASTERISK-24935
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> res_pjsip_phoneprov_provider was leaking references to phoneprov objects due 
> to a missing OBJ_NODATA in an ao2_callback in load_users().  Rather than 
> adding the OBJ_NODATA, I changed load_users to use a more straightforward 
> ao2_iterator.  This plugged the leak but exposed an unload order issue 
> between res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip.
> 
> res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then 
> res_pjsip.  Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery 
> instance, when it unloads, it's objects are still in the sorcery instance.  
> When res_pjsip unloads, it destroys all its objects including 
> res_pjsip_phoneprov_provider's.  The phoneprov destructor then attempts to 
> unregister the extension from res_phoneprov but because res_phoneprov is 
> already cleaned up, its users container is gone and we get a FRACK.
> 
> Simple solution, check for the NULL users container before attempting to 
> remove the entry. Duh.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_phoneprov_provider.c 434447 
>   branches/13/res/res_phoneprov.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4608/diff/
> 
> 
> Testing
> ---
> 
> Ran tests/res_phoneprov/res_phoneprov_provider.  No leaks in 
> res_pjsip_phoneprov_provider and no FRACKs.
> 
> 
> 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] 4596: res_pjsip_phoneprov_provider: Fix leaked OBJ_MULTIPLE iterator (2nd try)

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4596/#review15144
---


Can you update the description with the current analysis of what is going on 
and how this fixes it? The load order, the unload order, what happens when, 
that sort of thing.

- Joshua Colp


On April 8, 2015, 6:09 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4596/
> ---
> 
> (Updated April 8, 2015, 6:09 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Bugs: ASTERISK-24935
> https://issues.asterisk.org/jira/browse/ASTERISK-24935
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Original issue:  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.
> 
> Unfortunately, this highlighted a module unload order issue where 
> res_phoneprov and res_pjsip_phoneprov_provider were unloading before 
> res_pjsip_config_wizard, which needed them.
> 
> res_pjsip_config_wizard is itself a sorcery wizard so there are some 
> complexities to it's load order (it's long story) but I've removed the 
> GLOBAL_SYMBOLS flag from res_pjsip_config_wizard so it loads later and 
> unloads earlier and also triggered a reload of res_pjsip_phoneprov_provider.  
> Now they load and unload in the correct order.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_phoneprov_provider.c 434423 
>   branches/13/res/res_pjsip_config_wizard.c 434423 
> 
> Diff: https://reviewboard.asterisk.org/r/4596/diff/
> 
> 
> Testing
> ---
> 
> Checked load/unload order and make sure there were no FRACKs on unload.
> 
> 
> 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] 4601: Two simple patches for the price of one.

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4601/
---

(Updated April 9, 2015, 10:54 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 434508


Repository: Asterisk


Description
---

chan_iax2.c: Fix ref leak in iax2_request().

* Increased warning message format capability string buffer size in
iax2_request().

bridge_native_rtp.c: Defer allocation and check if it fails in 
native_rtp_bridge_compatible().


Diffs
-

  /branches/13/channels/chan_iax2.c 434430 
  /branches/13/bridges/bridge_native_rtp.c 434430 

Diff: https://reviewboard.asterisk.org/r/4601/diff/


Testing
---

Items found while researching code for ASTERISK-24841.  It compiles.  Warm 
fuzzy because it was part of code put in while testing overall code for 
ASTERISK-24841.


Thanks,

rmudgett

-- 
_
-- 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] 4590: Optional API does not work for sources that are both provider and user of optional API's.

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4590/#review15143
---

Ship it!


Ship It!

- Joshua Colp


On April 8, 2015, 6:21 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4590/
> ---
> 
> (Updated April 8, 2015, 6:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-17608
> https://issues.asterisk.org/jira/browse/ASTERISK-17608
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> OPTIONAL_API has conditionals to define AST_OPTIONAL_API and 
> AST_OPTIONAL_API_ATTR differently based on if AST_API_MODULE is defined.  
> Unfortunately this is inside the "#ifndef __ASTERISK_OPTIONAL_API_H" include 
> protection block, so only the first status of AST_API_MODULE is respected.  
> For example res_monitor is a provider, but uses beep.h (func_periodic_hook).  
> This makes func_periodic_hook non-optional to res_monitor.
> 
> The patch applies cleanly to 13 and fixes the issue, but I'm not sure this 
> should be fixed in 13.  Although there is no ABI change, it is a change in 
> behaviour for sources that #include optional_api.h.  If this is wanted for 13 
> please let me know, otherwise I will follow up with a patch for 13 to make 
> res_monitor require func_periodic_hook.
> 
> 
> Diffs
> -
> 
>   /trunk/include/asterisk/optional_api.h 434423 
> 
> Diff: https://reviewboard.asterisk.org/r/4590/diff/
> 
> 
> Testing
> ---
> 
> Verified I could load res_monitor with or without func_periodic_hook.  Ran a 
> couple testsuite tests.
> 
> 
> 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] 4476: new res_pjsip module to identify endpoint for an incoming call with a trunk that has outbound registration.

[Dmitriy Serov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4476/
---

(Updated April 9, 2015, 1:02 p.m.)


Status
--

This change has been discarded.


Review request for Asterisk Developers, Matt Jordan and rnewton.


Bugs: ASTERISK-24852
https://issues.asterisk.org/jira/browse/ASTERISK-24852


Repository: Asterisk


Description
---

The main task: to find the right endpoint for an incoming call with a trunk 
that has outbound registration.

Simple example (a piece of pjsip.conf):
[trunk1]
type=registration
transport=udp-transport
outbound_auth=trunk1
server_uri=sip:sip.example.com
client_uri=sip:us...@sip.example.com
contact_user=trunk1-in

[trunk2]
type=registration
transport=udp-transport
outbound_auth=trunk2
server_uri=sip:sip.example.com
client_uri=sip:us...@sip.example.com
contact_user=trunk2-in

[trunk1-in]
type=endpoint
context=from-trunk
disallow=all
allow=ulaw
outbound_auth=trunk1
aors=trunk1

[trunk2-in]
type=endpoint
context=from-trunk
disallow=all
allow=ulaw
outbound_auth=trunk2
aors=trunk2

trunk1, trunk2 - outbound registrations to EXTERNAL sip server sip.example.com. 
One server and two registrations.
; "contact_user=" sets the SIP contact header's user portion of the SIP URI 
this will affect the extension reached in dialplan when the far end calls you 
at this registration.
In example option has values "trunk1-in" and "trink2-in".

Case: random user ad...@sip.example.com calling to us...@sip.example.com.
External sip server redirect call to my server with packet:
INVITE sip:trunk2-in@8.8.8.8:5060 SIP/2.0
From: "PhonerLite" 
;tag=5F39A540-782390-DE41886A_kmbdctn-8A47
To: 

This invite can be:
- anonymous. Very bad.
- identified by IP. Config has two endpoints with same IP.
- identified by username From. It cannot identify by "admin" (random)

res_pjsip_endpoint_identifier_request_user helps to identify this invite by uri 
username in request line (contact_user in registration).


Diffs
-

  /trunk/res/res_pjsip_endpoint_identifier_request_user.c PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/4476/diff/


Testing
---


Thanks,

Dmitriy Serov

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4483: Separate QoS settings for trunk packets

[Y Ateya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4483/
---

(Updated April 9, 2015, 7:27 p.m.)


Status
--

This change has been discarded.


Review request for Asterisk Developers and mattjordan.


Bugs: ASTERISK-24866
https://issues.asterisk.org/jira/browse/ASTERISK-24866


Repository: Asterisk


Description
---

Currently the packet priority is assigned to all packets sent from IAX socket; 
Which minimizes the benefit of having packet priority.

This patch adds separate qos settings for IAX trunk packets.

Summary:
 - Added trunk_cos and trunk_tos configuraiton variables (similar to cos and 
tos) to set qos values for trunk packets.
 - Added dynamic_qos configuration variables (true/false) to enable/disable 
changing qos dynamically. (default disabled).
 - Before calling ast_send (and if dynamic_qos is enabled) update qos settings 
of the socket (if it needs change only).
 - Lock a mutex before setting qos and unlock it after ast_send. To prevent 
thread1 setting qos, thread two setting qos and send, then thread1 uses thread2 
qos.

Notes:
 - Protecting `qos_dynamic_enabled` in reload configurations complicated 
locking mechanism a little bit.
 - More qos classes can be added to different packet types later.


Diffs
-

  trunk/channels/chan_iax2.c 432806 

Diff: https://reviewboard.asterisk.org/r/4483/diff/


Testing
---

Run 100 calls (over IAX trunk) between client and server. Checked that:
 - If cos, tos are set but dynamic_qos is disabled, tos and cos values are used.
 - If tos, cos, dynamic_qos, trunk_cos and trunk_tos are set; trunk packets 
uses trunk_cos/trunk_tos while all other packets use cos/tos.


Thanks,

Y Ateya

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4605: translate.c: Only select audio codecs to determine the best translation choice.

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4605/#review15161
---

Ship it!


Ship It!

- Mark Michelson


On April 9, 2015, 4:50 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4605/
> ---
> 
> (Updated April 9, 2015, 4:50 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21777, ASTERISK-24380 and ASTERISK-24841
> https://issues.asterisk.org/jira/browse/ASTERISK-21777
> https://issues.asterisk.org/jira/browse/ASTERISK-24380
> https://issues.asterisk.org/jira/browse/ASTERISK-24841
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Given a source capability of h264 and ulaw, a destination capability of
> h264 and g722 then ast_translator_best_choice() would pick h264 as the
> best choice even though h264 is a video codec and Asterisk only supports
> translation of audio codecs.  When the audio starts flowing, there are
> warnings about a codec mismatch when the channel tries to write a frame to
> the peer.
> 
> * Made ast_translator_best_choice() only select audio codecs.
> 
> * Restore a check in channel.c:set_format() lost after v1.8 to prevent
> trying to set a non-audio codec.
> 
> This is an intermediate patch for a series of patches aimed at improving
> translation path choices for ASTERISK-24841.
> 
> This patch is a partial fix for ASTERISK-21777 as the v11 version of
> ast_translator_best_choice() does the same thing.  However, chan_sip
> somehow sets the nativeformats capabilities to just h264 or empty.
> 
> 
> This patch will be backported to v11 as a partial fix for ASTERISK-21777.
> 
> 
> Diffs
> -
> 
>   /branches/13/main/translate.c 434447 
>   /branches/13/main/channel.c 434447 
> 
> Diff: https://reviewboard.asterisk.org/r/4605/diff/
> 
> 
> Testing
> ---
> 
> Modified chan_pjsip.c:chan_pjsip_new() to force inclusion of the h264
> video codec in the native capabilities.  Without the patch I get the path
> translation warnings.  With the patch I don't get the warnings.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-- 
_
-- 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] 4606: chan_sip: make progressinband default to no

[Kevin Harwell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4606/
---

Review request for Asterisk Developers.


Bugs: ASTERISK-24835
https://issues.asterisk.org/jira/browse/ASTERISK-24835


Repository: Asterisk


Description
---

After the "progressinband" value setting of "never" was updated to never send a 
183 this separated its use from the "no" value. Since "never" was the default, 
but most users probably expect "no" this patch updates the default for the 
"progressinband" setting to "no".


Diffs
-

  branches/13/configs/samples/sip.conf.sample 434508 
  branches/13/channels/sip/include/sip.h 434508 
  branches/13/channels/chan_sip.c 434508 
  branches/13/CHANGES 434508 

Diff: https://reviewboard.asterisk.org/r/4606/diff/


Testing
---

Started Asterisk and loaded chan_sip with the new default value for 
progressinband. Check to make sure that is what it was set to. Changed the 
setting to the other values and reloaded/checked between each to make sure 
those got set correctly as well.


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] 4598: Refactor duplicated DNS routines into common sections

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4598/
---

(Updated April 9, 2015, 10:50 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 434490


Repository: Asterisk


Description
---

Josh worked on SRV in one branch, and I worked on NAPTR in a separate branch. 
Independently we kept coming to realizations that something that one of us had 
developed independently would be needed by the other person. We decided to 
simply have copies of common functionality in our branches. After merging, we 
would perform a refactor to remove duplication.

This changeset introduces no new DNS functionality. Instead, it takes some 
duplicated code and places them into common areas of the DNS core.


Diffs
-

  /trunk/tests/test_dns_srv.c 434218 
  /trunk/tests/test_dns_naptr.c 434218 
  /trunk/main/dns_test.c PRE-CREATION 
  /trunk/main/dns_srv.c 434218 
  /trunk/main/dns_naptr.c 434218 
  /trunk/main/dns_core.c 434218 
  /trunk/include/asterisk/dns_test.h PRE-CREATION 
  /trunk/include/asterisk/dns_internal.h 434218 

Diff: https://reviewboard.asterisk.org/r/4598/diff/


Testing
---

All DNS unit tests continue to pass.


Thanks,

Mark Michelson

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4607: bridge_softmix.c, channel.c: Minor code simplification and cleanup.

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4607/#review15165
---

Ship it!


Ship It!

- Joshua Colp


On April 9, 2015, 5:04 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4607/
> ---
> 
> (Updated April 9, 2015, 5:04 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> * Made code easier to follow in bridge_softmix.c:analyse_softmix_stats()
> and made some debug messages more helpful.
> 
> * Made some debug and warning messages more helpful in
> channel.c:set_format().
> 
> 
> Diffs
> -
> 
>   /branches/13/main/channel.c 434509 
>   /branches/13/bridges/bridge_softmix.c 434509 
> 
> Diff: https://reviewboard.asterisk.org/r/4607/diff/
> 
> 
> Testing
> ---
> 
> Items found while researching code for ASTERISK-24841.  It compiles.  Warm 
> fuzzy because it was part of code put in while testing overall code for 
> ASTERISK-24841.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-- 
_
-- 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] 4499: Support in dialog OPTIONS

[Matt Jordan]

> On March 23, 2015, 3:01 p.m., Matt Jordan wrote:
> > Thanks for the patch! I've clicked the Ship It button, although the same 
> > statement about requiring tests for things going into Asterisk 13 that I 
> > made on the DTMF review applies here as well.
> > 
> > In this particular case, a test for this patch should be done using SIPp, 
> > as it is pretty easy to construct an inbound INVITE request and put an 
> > OPTION request in-dialog with that INVITE request.
> > 
> > Most of the tests in channels/pjsip use SIPp to drive the tests, and so 
> > there is a lot of material to base a test on. We also have sample SIPp 
> > scenarios to use as a template in the contrib/sipp folder.
> > 
> > If you have any questions about where to start with that, please don't 
> > hesitate to ask on the asterisk-dev mailing list/#asterisk-dev.
> 
> Joshua Colp wrote:
> A testsuite test has now been published for review at 
> https://gerrit.asterisk.org/#/c/37/

Since the test is up, I'm going to go ahead and commit this.

yaron: Thanks a lot for the patch!


- Matt


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4499/#review14774
---


On March 18, 2015, 4:01 a.m., yaron nahum wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4499/
> ---
> 
> (Updated March 18, 2015, 4:01 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24862
> https://issues.asterisk.org/jira/browse/ASTERISK-24862
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Respond to OPTIONS message sent on an existing dialog with 200OK. 
> This feature is vital in order to interoperate with some switches that send 
> OPTIONS message periodically per active call to make sure it is still alive. 
> Not responding would cause the switch to disconnect the call. 
> This functionality used to work on the old SIP channel, but was not 
> implemented on PJSIP.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_pjsip_dlg_options.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4499/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> yaron nahum
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4536: iax2_poke_noanswer expiration timer too short

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4536/#review15145
---

Ship it!


Ship It!

- Matt Jordan


On April 7, 2015, 3:38 p.m., Y Ateya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4536/
> ---
> 
> (Updated April 7, 2015, 3:38 p.m.)
> 
> 
> Review request for Asterisk Developers and rnewton.
> 
> 
> Bugs: ASTERISK-24894
> https://issues.asterisk.org/jira/browse/ASTERISK-24894
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Increase POKE retry window from DEFAULT_MAXMS * 2 (4 seconds) to bigger 
> number (derived from qualify time; which control POKE retry time).
> 
> 
> Diffs
> -
> 
>   trunk/channels/chan_iax2.c 432806 
> 
> Diff: https://reviewboard.asterisk.org/r/4536/diff/
> 
> 
> Testing
> ---
> 
> - Tried test with multiple qualify values (2 and 10 seconds).
> - Tried test with 100% packets loss to ensure that when a POKE packet is 
> dropped it will be retried couple of time before declaring client 
> disconnected.
> 
> 
> Thanks,
> 
> Y Ateya
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev