[asterisk-dev] [Code Review] 4317: Testsuite: PJSIP remote attended transfer test

[Mark Michelson]

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

Review request for Asterisk Developers.


Repository: testsuite


Description
---

This is a testsuite test that performs a remote attended transfer. The test is 
a bit odd, but the test-config.yaml should do a decent job of explaining how 
this works.

The test could be structured into a more strict state machine style, but given 
that this is just a test, I didn't want to waste extra cycles on that. As it 
is, the test passes, and the comments should do a decent job of explaining 
what's going on.

The setup is a bit contrived, but it accurately tests a remote attended 
transfer scenario, so it works :)


Diffs
-

  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/tests.yaml
 6177 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/transfer.py
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/test-config.yaml
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/configs/ast2/pjsip.conf
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/configs/ast2/extensions.conf
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/configs/ast1/pjsip.conf
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/pjsip/transfers/attended_transfer/nominal/callee_remote/configs/ast1/extensions.conf
 PRE-CREATION 
  /asterisk/trunk/lib/python/asterisk/pjsua_mod.py 6177 

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


Testing
---

The test passes with the patch from /r/4296. Without that patch, the test fails.


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] 4285: Bug fixes for ARI Originate/Continue with label support (Continuation of /r/4101)

[Joshua Colp]

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

Ship it!


Ship It!

- Joshua Colp


On Jan. 6, 2015, 10:21 p.m., Mark Michelson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4285/
> ---
> 
> (Updated Jan. 6, 2015, 10:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24412
> https://issues.asterisk.org/jira/browse/ASTERISK-24412
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> See /r/4101 for the initial review uploaded by Nir Simionivich to understand 
> the need for the ability to originate/continue to a labeled dialplan priority.
> 
> When writing the tests in /r/4284 for continuation/origination, I ran into 
> some problems, and so I have updated the original patch to fix these issues. 
> Here are the modifications to the original patch:
> 
> * In continuation code, we get a channel snapshot from the stasis_app_control 
> instead of getting an actual channel instance. This is because 
> pbx_findlabel_extension doesn't actually use the channel for anything, so the 
> channel isn't necessary.
> * In both continuation and origination code, do not pass the input context to 
> pbx_findlabel_extension. If no context is specified, this causes a crash. 
> Instead, determine the intended context beforehand and pass that context into 
> pbx_findlabel_extension.
> * This is not a fault of the previous patch, but there were code paths that 
> resulted in some unexpected behavior in continuation. Since continuation 
> states that context, extension, priority, and label are all optional, but 
> didn't really do anything to make sense of what should happen when one or 
> more are omitted, the code has been updated to make sure that a sane default 
> is used when one or more of these are omitted.
> * I have updated the apiVersion in rest-api/resources.json to "1.7.0" since 
> this change introduces backwards-compatible new functionality. QUESTION: Is 
> this version bump required in other .json files as well?
> * I have updated CHANGES to indicate the new functionality
> 
> 
> Diffs
> -
> 
>   /branches/13/rest-api/resources.json 430178 
>   /branches/13/rest-api/api-docs/channels.json 430178 
>   /branches/13/res/res_ari_channels.c 430178 
>   /branches/13/res/ari/resource_channels.c 430178 
>   /branches/13/res/ari/resource_channels.h 430178 
>   /branches/13/CHANGES 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4285/diff/
> 
> 
> Testing
> ---
> 
> See /r/4284 for tests.
> 
> 
> 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] 4285: Bug fixes for ARI Originate/Continue with label support (Continuation of /r/4101)

[Mark Michelson]

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

(Updated Jan. 6, 2015, 10:21 p.m.)


Review request for Asterisk Developers.


Changes
---

Added same error message to origination as well.


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


Repository: Asterisk


Description
---

See /r/4101 for the initial review uploaded by Nir Simionivich to understand 
the need for the ability to originate/continue to a labeled dialplan priority.

When writing the tests in /r/4284 for continuation/origination, I ran into some 
problems, and so I have updated the original patch to fix these issues. Here 
are the modifications to the original patch:

* In continuation code, we get a channel snapshot from the stasis_app_control 
instead of getting an actual channel instance. This is because 
pbx_findlabel_extension doesn't actually use the channel for anything, so the 
channel isn't necessary.
* In both continuation and origination code, do not pass the input context to 
pbx_findlabel_extension. If no context is specified, this causes a crash. 
Instead, determine the intended context beforehand and pass that context into 
pbx_findlabel_extension.
* This is not a fault of the previous patch, but there were code paths that 
resulted in some unexpected behavior in continuation. Since continuation states 
that context, extension, priority, and label are all optional, but didn't 
really do anything to make sense of what should happen when one or more are 
omitted, the code has been updated to make sure that a sane default is used 
when one or more of these are omitted.
* I have updated the apiVersion in rest-api/resources.json to "1.7.0" since 
this change introduces backwards-compatible new functionality. QUESTION: Is 
this version bump required in other .json files as well?
* I have updated CHANGES to indicate the new functionality


Diffs (updated)
-

  /branches/13/rest-api/resources.json 430178 
  /branches/13/rest-api/api-docs/channels.json 430178 
  /branches/13/res/res_ari_channels.c 430178 
  /branches/13/res/ari/resource_channels.c 430178 
  /branches/13/res/ari/resource_channels.h 430178 
  /branches/13/CHANGES 430178 

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


Testing
---

See /r/4284 for tests.


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] 4285: Bug fixes for ARI Originate/Continue with label support (Continuation of /r/4101)

[Mark Michelson]

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

(Updated Jan. 6, 2015, 10:20 p.m.)


Review request for Asterisk Developers.


Changes
---

Changed the error message Josh pointed out to simply point out that the entered 
priority label is invalid.


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


Repository: Asterisk


Description
---

See /r/4101 for the initial review uploaded by Nir Simionivich to understand 
the need for the ability to originate/continue to a labeled dialplan priority.

When writing the tests in /r/4284 for continuation/origination, I ran into some 
problems, and so I have updated the original patch to fix these issues. Here 
are the modifications to the original patch:

* In continuation code, we get a channel snapshot from the stasis_app_control 
instead of getting an actual channel instance. This is because 
pbx_findlabel_extension doesn't actually use the channel for anything, so the 
channel isn't necessary.
* In both continuation and origination code, do not pass the input context to 
pbx_findlabel_extension. If no context is specified, this causes a crash. 
Instead, determine the intended context beforehand and pass that context into 
pbx_findlabel_extension.
* This is not a fault of the previous patch, but there were code paths that 
resulted in some unexpected behavior in continuation. Since continuation states 
that context, extension, priority, and label are all optional, but didn't 
really do anything to make sense of what should happen when one or more are 
omitted, the code has been updated to make sure that a sane default is used 
when one or more of these are omitted.
* I have updated the apiVersion in rest-api/resources.json to "1.7.0" since 
this change introduces backwards-compatible new functionality. QUESTION: Is 
this version bump required in other .json files as well?
* I have updated CHANGES to indicate the new functionality


Diffs (updated)
-

  /branches/13/rest-api/resources.json 430178 
  /branches/13/rest-api/api-docs/channels.json 430178 
  /branches/13/res/res_ari_channels.c 430178 
  /branches/13/res/ari/resource_channels.c 430178 
  /branches/13/res/ari/resource_channels.h 430178 
  /branches/13/CHANGES 430178 

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


Testing
---

See /r/4284 for tests.


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] 4311: res_pjsip: make it unloadable

[George Joseph]

> On Jan. 6, 2015, 1:34 p.m., George Joseph wrote:
> > So, I applied the patch but am having trouble seeing what this does in real 
> > life.  res_pjsip can only load by itself if there's no configuration.  Even 
> > a minimal one requires res_pjsip_session and 
> > res_pjsip_outbound_authenticator_digest.  So in the end, you have to reload 
> > with no config, then start unloading modules.  Why not just restart with 
> > the stack noloaded in modules.conf?
> >
> 
> George Joseph wrote:
> Also you might want to update MODULEINFO to indicate that 
> res_sorcery_config, res_sorcery_memory and res_sorcery_astdb are require to 
> load res_pjsip in the first place.
>
> 
> George Joseph wrote:
> Unloading res_pjsip_outbound_registration segfaults if there's a 
> registration defined. :)
> 
> Kevin Harwell wrote:
> [My fault for not explaining the reasoning behind the patch better] This 
> patch is the first step (should hopefully be followed by another/others at 
> some point) in allowing res_pjsip and the modules that depend on it to be 
> unloadable.  At this time, as you noted, res_pjsip and some of the modules 
> that depend on res_pjsip cannot be unloaded without causing problems of some 
> sort.
> 
> The goal of this patch is to get res_pjsip and only res_pjsip to be able 
> to unload successfully and/or shutdown without incident (crashes, leaks, 
> etc...). Other dependent modules will still cause problems on unload 
> (res_pjsip_session, res_pjsip_pubsub, etc...).
> 
> So yeah basically I just made sure, with the patch applied, that 
> res_pjsip (with no other dependent modules loaded) could be succesfully 
> unloaded and Asterisk could shutdown without any leaks or crashes that 
> pertained directly to res_pjsip.

Ah, sounds good.  I did test that it was indeed unloadable.


- George


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


On Jan. 6, 2015, 2:30 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 6, 2015, 2:30 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430244 
>   branches/13/res/res_pjsip/pjsip_options.c 430244 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430244 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430244 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430244 
>   branches/13/res/res_pjsip/location.c 430244 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430244 
>   branches/13/res/res_pjsip/config_transport.c 430244 
>   branches/13/res/res_pjsip/config_auth.c 430244 
>   branches/13/res/res_pjsip.c 430244 
>   branches/13/main/stasis_message_router.c 430244 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[Kevin Harwell]

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

(Updated Jan. 6, 2015, 3:30 p.m.)


Review request for Asterisk Developers.


Changes
---

Renamed internal functions and updated moduleinfo with dependencies.


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


Repository: Asterisk


Description
---

The res_pjsip module was previously unloadable. With this patch it can now be 
unloaded.

This patch is based off the original patch on the issue by Corey Farrell with a 
few modifications. Removed a few changes not required to make the module 
unloadable and also fixed a bug that would cause asterisk to crash on unloading.


Diffs (updated)
-

  branches/13/res/res_pjsip/pjsip_outbound_auth.c 430244 
  branches/13/res/res_pjsip/pjsip_options.c 430244 
  branches/13/res/res_pjsip/pjsip_global_headers.c 430244 
  branches/13/res/res_pjsip/pjsip_distributor.c 430244 
  branches/13/res/res_pjsip/pjsip_configuration.c 430244 
  branches/13/res/res_pjsip/location.c 430244 
  branches/13/res/res_pjsip/include/res_pjsip_private.h 430244 
  branches/13/res/res_pjsip/config_transport.c 430244 
  branches/13/res/res_pjsip/config_auth.c 430244 
  branches/13/res/res_pjsip.c 430244 
  branches/13/main/stasis_message_router.c 430244 

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


Testing
---

Made it so res_pjsip was the only pjsip module loaded and then issued an unload 
and noted it unloaded successfully (also loaded/unloaded it several times from 
the CLI). Also when loaded and with REF_DEBUG enabled issued a "core stop 
gracefully" and made sure there were no ref leaks for the module.

Also tested unloading with other dependent pjsip modules loaded and noted that 
the module would not unload (as it should since dependencies are currently 
loaded). And then shutdown asterisk and made sure it did not crash or anything.

Started asterisk with nominal and off nominal module and pjsip configurations 
to make sure things behaved appropriately (no crashes and such) and then 
attempted to, or successfully unload the res_pjsip module. Also made sure 
Asterisk continued to shutdown without incident.


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] 4311: res_pjsip: make it unloadable

[Kevin Harwell]

> On Jan. 6, 2015, 2:34 p.m., George Joseph wrote:
> > So, I applied the patch but am having trouble seeing what this does in real 
> > life.  res_pjsip can only load by itself if there's no configuration.  Even 
> > a minimal one requires res_pjsip_session and 
> > res_pjsip_outbound_authenticator_digest.  So in the end, you have to reload 
> > with no config, then start unloading modules.  Why not just restart with 
> > the stack noloaded in modules.conf?
> >
> 
> George Joseph wrote:
> Also you might want to update MODULEINFO to indicate that 
> res_sorcery_config, res_sorcery_memory and res_sorcery_astdb are require to 
> load res_pjsip in the first place.
>
> 
> George Joseph wrote:
> Unloading res_pjsip_outbound_registration segfaults if there's a 
> registration defined. :)

[My fault for not explaining the reasoning behind the patch better] This patch 
is the first step (should hopefully be followed by another/others at some 
point) in allowing res_pjsip and the modules that depend on it to be 
unloadable.  At this time, as you noted, res_pjsip and some of the modules that 
depend on res_pjsip cannot be unloaded without causing problems of some sort.

The goal of this patch is to get res_pjsip and only res_pjsip to be able to 
unload successfully and/or shutdown without incident (crashes, leaks, etc...). 
Other dependent modules will still cause problems on unload (res_pjsip_session, 
res_pjsip_pubsub, etc...).

So yeah basically I just made sure, with the patch applied, that res_pjsip 
(with no other dependent modules loaded) could be succesfully unloaded and 
Asterisk could shutdown without any leaks or crashes that pertained directly to 
res_pjsip.


- Kevin


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


On Jan. 2, 2015, 1:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 1:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[George Joseph]

> On Jan. 6, 2015, 1:34 p.m., George Joseph wrote:
> > So, I applied the patch but am having trouble seeing what this does in real 
> > life.  res_pjsip can only load by itself if there's no configuration.  Even 
> > a minimal one requires res_pjsip_session and 
> > res_pjsip_outbound_authenticator_digest.  So in the end, you have to reload 
> > with no config, then start unloading modules.  Why not just restart with 
> > the stack noloaded in modules.conf?
> >
> 
> George Joseph wrote:
> Also you might want to update MODULEINFO to indicate that 
> res_sorcery_config, res_sorcery_memory and res_sorcery_astdb are require to 
> load res_pjsip in the first place.
>

Unloading res_pjsip_outbound_registration segfaults if there's a registration 
defined. :)


- George


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


On Jan. 2, 2015, 12:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 12:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[George Joseph]

> On Jan. 6, 2015, 1:34 p.m., George Joseph wrote:
> > So, I applied the patch but am having trouble seeing what this does in real 
> > life.  res_pjsip can only load by itself if there's no configuration.  Even 
> > a minimal one requires res_pjsip_session and 
> > res_pjsip_outbound_authenticator_digest.  So in the end, you have to reload 
> > with no config, then start unloading modules.  Why not just restart with 
> > the stack noloaded in modules.conf?
> >

Also you might want to update MODULEINFO to indicate that res_sorcery_config, 
res_sorcery_memory and res_sorcery_astdb are require to load res_pjsip in the 
first place.


- George


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


On Jan. 2, 2015, 12:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 12:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[George Joseph]

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


So, I applied the patch but am having trouble seeing what this does in real 
life.  res_pjsip can only load by itself if there's no configuration.  Even a 
minimal one requires res_pjsip_session and 
res_pjsip_outbound_authenticator_digest.  So in the end, you have to reload 
with no config, then start unloading modules.  Why not just restart with the 
stack noloaded in modules.conf?


- George Joseph


On Jan. 2, 2015, 12:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 12:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[Mark Michelson]

> On Jan. 5, 2015, 10:47 p.m., Mark Michelson wrote:
> > branches/13/res/res_pjsip/include/res_pjsip_private.h, lines 123-141
> > 
> >
> > I unnastand the need for these functions, but I feel like they could be 
> > named more appropriately.
> > 
> > First, due to a lack of proper namespacing in C, you have to be careful 
> > defining functions that are called pjsip_*. It is possible that such a 
> > function could be added to PJSIP itself and cause a conflict with us. Note 
> > that this applies to other functions besides the ones I've highlighted here.
> > 
> > Second, the names do not do a great job of differentiating between the 
> > ast_sip_* versions of the functions. This may be hard to describe in a 
> > brief function name.
> 
> Kevin Harwell wrote:
> How does sip_register_service_no_ref sound?  I'd like to avoid prefixing 
> it with "ast_" since that is usually reserved for exported functions and 
> these are internal.
> 
> if sip_register_blah is too generic then maybe internal_sip_register?  
> Thoughts/suggestions?

The "internal" version sounds fine to me.


- Mark


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


On Jan. 2, 2015, 7:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 7:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4296: PJSIP: Fix bugs and improve documentation of remote attended transfers

[Kevin Harwell]

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

Ship it!


Ship It!

- Kevin Harwell


On Jan. 6, 2015, 11:29 a.m., Mark Michelson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4296/
> ---
> 
> (Updated Jan. 6, 2015, 11:29 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24376
> https://issues.asterisk.org/jira/browse/ASTERISK-24376
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> ASTERISK-24376 was created because at SIPit there was confusion about how to 
> get remote attended transfers to work properly when using chan_pjsip.
> 
> To address this, I investigated the code and discerned what the intended way 
> to perform a remote attended transfer is. I documented my findings on the 
> following wiki page: 
> https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=31096903
> Please review the wiki page to ensure it makes sense.
> 
> While setting up a remote attended transfer, I came across a couple of minor 
> issues. The following bug fixes are attached on this review:
> * When sending an INVITE with a Replaces header, the Replaces header was 
> malformed. It had the word "Replaces" twice.
> * There was an error message that made it sound like external_replaces was a 
> context being searched for, when it's actually an extension. I changed the 
> log message to be more clear.
> 
> 
> Diffs
> -
> 
>   /branches/13/res/res_pjsip_refer.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4296/diff/
> 
> 
> Testing
> ---
> 
> I have manually tested this, and I've found that remote attended transfers 
> work as expected with the patch applied. I'm in the process of writing a 
> testsuite test but am fighting with PJSUA at the moment. Since I am going on 
> vacation for a couple of weeks, I felt I should go ahead and get this review 
> posted now. You'll see a testsuite review go up sometime at the start of 
> 2015. Happy Holidays!
> 
> 
> 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] 4311: res_pjsip: make it unloadable

[Kevin Harwell]

> On Jan. 5, 2015, 4:47 p.m., Mark Michelson wrote:
> > branches/13/res/res_pjsip/include/res_pjsip_private.h, lines 123-141
> > 
> >
> > I unnastand the need for these functions, but I feel like they could be 
> > named more appropriately.
> > 
> > First, due to a lack of proper namespacing in C, you have to be careful 
> > defining functions that are called pjsip_*. It is possible that such a 
> > function could be added to PJSIP itself and cause a conflict with us. Note 
> > that this applies to other functions besides the ones I've highlighted here.
> > 
> > Second, the names do not do a great job of differentiating between the 
> > ast_sip_* versions of the functions. This may be hard to describe in a 
> > brief function name.

How does sip_register_service_no_ref sound?  I'd like to avoid prefixing it 
with "ast_" since that is usually reserved for exported functions and these are 
internal.

if sip_register_blah is too generic then maybe internal_sip_register?  
Thoughts/suggestions?


- Kevin


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


On Jan. 2, 2015, 1:46 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> ---
> 
> (Updated Jan. 2, 2015, 1:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
> https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The res_pjsip module was previously unloadable. With this patch it can now be 
> unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with 
> a few modifications. Removed a few changes not required to make the module 
> unloadable and also fixed a bug that would cause asterisk to crash on 
> unloading.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> ---
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an 
> unload and noted it unloaded successfully (also loaded/unloaded it several 
> times from the CLI). Also when loaded and with REF_DEBUG enabled issued a 
> "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted 
> that the module would not unload (as it should since dependencies are 
> currently loaded). And then shutdown asterisk and made sure it did not crash 
> or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations 
> to make sure things behaved appropriately (no crashes and such) and then 
> attempted to, or successfully unload the res_pjsip module. Also made sure 
> Asterisk continued to shutdown without incident.
> 
> 
> 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] 4314: res_pjsip_mwi: Suppress another warning

[George Joseph]

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

(Updated Jan. 6, 2015, 11:52 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 430227


Repository: Asterisk


Description
---

When res_pjsip loads and an endpoint auto-subscribes a mailbox for mwi, if a 
contact hasn't registered yet, res_pjsip_mwi spits out a warning.  This is a 
perfectly normal situation though and doesn't require something as serious as a 
warning.  It's also self correcting. The device will start getting mwi as soon 
as it registers.

This patch changes the warning to a verb/3.


Diffs
-

  branches/13/res/res_pjsip_mwi.c 430220 

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


Testing
---


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] 4314: res_pjsip_mwi: Suppress another warning

[Matt Jordan]

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

Ship it!


Ship It!

- Matt Jordan


On Jan. 6, 2015, 10:29 a.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4314/
> ---
> 
> (Updated Jan. 6, 2015, 10:29 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When res_pjsip loads and an endpoint auto-subscribes a mailbox for mwi, if a 
> contact hasn't registered yet, res_pjsip_mwi spits out a warning.  This is a 
> perfectly normal situation though and doesn't require something as serious as 
> a warning.  It's also self correcting. The device will start getting mwi as 
> soon as it registers.
> 
> This patch changes the warning to a verb/3.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_mwi.c 430220 
> 
> Diff: https://reviewboard.asterisk.org/r/4314/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 4300: bridge_native_rtp: Change local/remote message from debug/2 to verb/4

[George Joseph]

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

(Updated Jan. 6, 2015, 11:46 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 430225


Repository: Asterisk


Description
---

Change the "Locally bridged"/"Remotely bridged" messages from debug/2 to verb/4.


Diffs
-

  branches/13/bridges/bridge_native_rtp.c 430163 

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


Testing
---

Messages appear at the correct verbose level in the CLI.


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] 4301: res_pjsip_outbound_registration: Add 'pjsip send register' command and update behavior of 'send unregister'

[George Joseph]

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

(Updated Jan. 6, 2015, 11:35 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers, opticron and Scott Griepentrog.


Changes
---

Committed in revision 430223


Repository: Asterisk


Description
---

The current behavior of 'pjsip send unregister' is to send the unregister 
(REGISTER with 0 exp) but let the next scheduled register proceed normally.  I 
don't think that's a good idea.  If you unregister, it should stay unregistered 
until you decide to start registrations again.  So this patch just adds a 
cancel_registration call to the current unregister_task to cancel the timer.

Of course, now you need  a way to start registration again so I've added a 
'pjsip send register' command that unregisters and cancels any existing 
registration (the same as send unregister), then sends an immediate 
registration and starts the timer back up again.

Both changes also ripple to AMI.  There's a new PJSIPRegister command.

There's no harm in calling either command repeatedly.  They don't care about 
the actual state.

I did discover a previously existing ref count leak for state though.  Every 
time asterisk sends a register, the count gets incremented but never 
decremented.  I'll look at this separately.


Diffs
-

  branches/13/res/res_pjsip_outbound_registration.c 430220 
  branches/13/CHANGES 430220 

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


Testing
---

The current tests/channels/pjsip/registration/outbound/unregister tests run 
fine.  I'm working on additional tests to exercise the register command.


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] 4296: PJSIP: Fix bugs and improve documentation of remote attended transfers

[Mark Michelson]

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

(Updated Jan. 6, 2015, 5:29 p.m.)


Review request for Asterisk Developers.


Changes
---

Null-terminate the buffer that PJSIP writes the Replaces header into.


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


Repository: Asterisk


Description
---

ASTERISK-24376 was created because at SIPit there was confusion about how to 
get remote attended transfers to work properly when using chan_pjsip.

To address this, I investigated the code and discerned what the intended way to 
perform a remote attended transfer is. I documented my findings on the 
following wiki page: 
https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=31096903
Please review the wiki page to ensure it makes sense.

While setting up a remote attended transfer, I came across a couple of minor 
issues. The following bug fixes are attached on this review:
* When sending an INVITE with a Replaces header, the Replaces header was 
malformed. It had the word "Replaces" twice.
* There was an error message that made it sound like external_replaces was a 
context being searched for, when it's actually an extension. I changed the log 
message to be more clear.


Diffs (updated)
-

  /branches/13/res/res_pjsip_refer.c 430178 

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


Testing
---

I have manually tested this, and I've found that remote attended transfers work 
as expected with the patch applied. I'm in the process of writing a testsuite 
test but am fighting with PJSUA at the moment. Since I am going on vacation for 
a couple of weeks, I felt I should go ahead and get this review posted now. 
You'll see a testsuite review go up sometime at the start of 2015. Happy 
Holidays!


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] 4305: pjsip cli: Fix sorting of contacts for 'pjsip list contacts'

[George Joseph]

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

(Updated Jan. 6, 2015, 11:28 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 430221


Repository: Asterisk


Description
---

For some reason I was using a hash container instead of a list to gather the 
contacts for 'pjsip list/show contacts' so even though I had a sort function, 
the output wasn't sorted.  This patch just changes the hash container to a list 
container and the contacts now appear sorted in the CLI.


Diffs
-

  branches/13/res/res_pjsip/location.c 430163 

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


Testing
---

Eyeballed contact list before and after to confirm they were now sorted.


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] 4314: res_pjsip_mwi: Suppress another warning

[George Joseph]

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

(Updated Jan. 6, 2015, 9:29 a.m.)


Review request for Asterisk Developers.


Changes
---

Using LOG_NOTICE now.


Repository: Asterisk


Description
---

When res_pjsip loads and an endpoint auto-subscribes a mailbox for mwi, if a 
contact hasn't registered yet, res_pjsip_mwi spits out a warning.  This is a 
perfectly normal situation though and doesn't require something as serious as a 
warning.  It's also self correcting. The device will start getting mwi as soon 
as it registers.

This patch changes the warning to a verb/3.


Diffs (updated)
-

  branches/13/res/res_pjsip_mwi.c 430220 

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


Testing
---


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] 4314: res_pjsip_mwi: Suppress another warning

[George Joseph]

> On Jan. 5, 2015, 5:11 p.m., Mark Michelson wrote:
> > I'm not sure I agree with this change. This can happen, like you said, due 
> > to MWI being indicated before an eventual registration. However, this can 
> > also happen due to a misconfiguration (either in Asterisk or the endpoint 
> > the MWI is being sent to) and not end up fixing itself without intervening. 
> > In the case of a misconfiguration, a warning goes a lot further towards 
> > pointing out the issue than a verbose message.
> > 
> > On the one hand, I can understand the annoyance in seeing a warning when 
> > things actually are fine, but I also don't want to withhold a message when 
> > something is genuinely wrong. I wonder if there is some sort of compromise 
> > that can be had here.
> 
> George Joseph wrote:
> My feeling is that remote misconfiguration shouldn't cause warnings or 
> errors but I don't see a way to tell the difference here.  We need a new log 
> level like LOG_CONFIG :).
> 
> If no one has an alternative by tomorrow, I'll discard the review.
> 
> Mark Michelson wrote:
> Well, maybe a NOTICE is a decent compromise? Default logger.conf has 
> NOTICEs appear in the CLI and most logs, but it doesn't carry the same tone 
> of emergency that a warning or error would.

Works for me.


- George


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


On Jan. 2, 2015, 3:41 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4314/
> ---
> 
> (Updated Jan. 2, 2015, 3:41 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When res_pjsip loads and an endpoint auto-subscribes a mailbox for mwi, if a 
> contact hasn't registered yet, res_pjsip_mwi spits out a warning.  This is a 
> perfectly normal situation though and doesn't require something as serious as 
> a warning.  It's also self correcting. The device will start getting mwi as 
> soon as it registers.
> 
> This patch changes the warning to a verb/3.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_mwi.c 430163 
> 
> Diff: https://reviewboard.asterisk.org/r/4314/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 4314: res_pjsip_mwi: Suppress another warning

[Mark Michelson]

> On Jan. 6, 2015, 12:11 a.m., Mark Michelson wrote:
> > I'm not sure I agree with this change. This can happen, like you said, due 
> > to MWI being indicated before an eventual registration. However, this can 
> > also happen due to a misconfiguration (either in Asterisk or the endpoint 
> > the MWI is being sent to) and not end up fixing itself without intervening. 
> > In the case of a misconfiguration, a warning goes a lot further towards 
> > pointing out the issue than a verbose message.
> > 
> > On the one hand, I can understand the annoyance in seeing a warning when 
> > things actually are fine, but I also don't want to withhold a message when 
> > something is genuinely wrong. I wonder if there is some sort of compromise 
> > that can be had here.
> 
> George Joseph wrote:
> My feeling is that remote misconfiguration shouldn't cause warnings or 
> errors but I don't see a way to tell the difference here.  We need a new log 
> level like LOG_CONFIG :).
> 
> If no one has an alternative by tomorrow, I'll discard the review.

Well, maybe a NOTICE is a decent compromise? Default logger.conf has NOTICEs 
appear in the CLI and most logs, but it doesn't carry the same tone of 
emergency that a warning or error would.


- Mark


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


On Jan. 2, 2015, 10:41 p.m., George Joseph wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4314/
> ---
> 
> (Updated Jan. 2, 2015, 10:41 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When res_pjsip loads and an endpoint auto-subscribes a mailbox for mwi, if a 
> contact hasn't registered yet, res_pjsip_mwi spits out a warning.  This is a 
> perfectly normal situation though and doesn't require something as serious as 
> a warning.  It's also self correcting. The device will start getting mwi as 
> soon as it registers.
> 
> This patch changes the warning to a verb/3.
> 
> 
> Diffs
> -
> 
>   branches/13/res/res_pjsip_mwi.c 430163 
> 
> Diff: https://reviewboard.asterisk.org/r/4314/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 4315: AMI: Make AMI actions that generate event lists consistent.

[Kevin Harwell]

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


Since we are bumping the API should this be mentioned in the UPGRADE file?


/branches/13/include/asterisk/manager.h


If listflag is no longer used do we want to deprecate this function and 
have a new one without the parameter.



/branches/13/include/asterisk/manager.h


Just a suggestion, but using a variable argument list you could combine 
these two functions.


- Kevin Harwell


On Jan. 2, 2015, 6:42 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4315/
> ---
> 
> (Updated Jan. 2, 2015, 6:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24049
> https://issues.asterisk.org/jira/browse/ASTERISK-24049
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> * Made the following AMI actions use list API calls for consistency:
> Agents
> BridgeInfo
> BridgeList
> BridgeTechnologyList
> ConfbridgeLIst
> ConfbridgeLIstRooms
> CoreShowChannels
> DAHDIShowChannels
> DBGet
> DeviceStateList
> ExtensionStateList
> FAXSessions
> Hangup
> IAXpeerlist
> IAXpeers
> IAXregistry
> MeetmeList
> MeetmeListRooms
> MWIGet
> ParkedCalls
> Parkinglots
> PJSIPShowEndpoint
> PJSIPShowEndpoints
> PJSIPShowRegistrationsInbound
> PJSIPShowRegistrationsOutbound
> PJSIPShowResourceLists
> PJSIPShowSubscriptionsInbound
> PJSIPShowSubscriptionsOutbound
> PresenceStateList
> PRIShowSpans
> QueueStatus
> QueueSummary
> ShowDialPlan
> SIPpeers
> SIPpeerstatus
> SIPshowregistry
> SKINNYdevices
> SKINNYlines
> Status
> VoicemailUsersList
> 
> * Incremented the AMI version to 2.7.0.
> 
> * Changed astman_send_listack() to not use the listflag parameter and
> always set the value to "Start" so the start capitalization is consistent.
> i.e., The FAXSessions used "Start" while the rest of the system used
> "start".  The corresponding complete event always used "Complete".
> 
> * Fixed ami_show_resource_lists() "PJSIPShowResourceLists" to output the
> AMI ActionID for all of its list events.
> 
> * Fixed off-nominal AMI protocol error in manager_bridge_info(),
> manager_parking_status_single_lot(), and
> manager_parking_status_all_lots().  Use of astman_send_error() after
> responding to the original AMI action request violates the action response
> pattern by sending two responses.
> 
> * Fixed minor protocol error in action_getconfig() when no requested
> categories are found.  Each line needs to be formatted as "Header: text".
> 
> * Fixed off-nominal memory leak in manager_build_parked_call_string().
> 
> * Eliminated unnecessary use of RAII_VAR() in ami_subscription_detail().
> 
> 
> Diffs
> -
> 
>   /branches/13/res/res_pjsip_registrar.c 430178 
>   /branches/13/res/res_pjsip_pubsub.c 430178 
>   /branches/13/res/res_pjsip_outbound_registration.c 430178 
>   /branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   /branches/13/res/res_mwi_external_ami.c 430178 
>   /branches/13/res/res_manager_presencestate.c 430178 
>   /branches/13/res/res_manager_devicestate.c 430178 
>   /branches/13/res/res_fax.c 430178 
>   /branches/13/res/parking/parking_manager.c 430178 
>   /branches/13/main/pbx.c 430178 
>   /branches/13/main/manager_bridges.c 430178 
>   /branches/13/main/manager.c 430178 
>   /branches/13/main/db.c 430178 
>   /branches/13/main/bridge.c 430178 
>   /branches/13/include/asterisk/manager.h 430178 
>   /branches/13/channels/chan_skinny.c 430178 
>   /branches/13/channels/chan_sip.c 430178 
>   /branches/13/channels/chan_iax2.c 430178 
>   /branches/13/channels/chan_dahdi.c 430178 
>   /branches/13/apps/app_voicemail.c 430178 
>   /branches/13/apps/app_queue.c 430178 
>   /branches/13/apps/app_meetme.c 430178 
>   /branches/13/apps/app_confbridge.c 430178 
>   /branches/13/apps/app_agent_pool.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4315/diff/
> 
> 
> Testing
> ---
> 
> Issued all of the AMI actions listed above to verify that the output was 
> consistent.
> 
> 
> 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] 4256: testsuite: check for channel leak on failed blonde transfer

[Kevin Harwell]

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

Ship it!


I'll go ahead and ship it as this looks good to me, but you still have an open 
issue so not sure if you want to commit this until that gets answered/resolved.

- Kevin Harwell


On Dec. 31, 2014, 12:01 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4256/
> ---
> 
> (Updated Dec. 31, 2014, 12:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24513
> https://issues.asterisk.org/jira/browse/ASTERISK-24513
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This test starts an attended transfer, converts to blonde mode by hanging up 
> the transferer, and then fails the transfer by hanging up the transferee.  
> Then after allowing the recall attempt to complete, checks to insure that 
> there are not leaked channels.
> 
> Improvements to channel_test_condition: count the actual channels listed in 
> "core show channels" output to check for leaks.  Also added unittest.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/bridge/tests.yaml 6149 
>   /asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml 
> PRE-CREATION 
>   
> /asterisk/trunk/tests/bridge/atxfer_fail_blonde/configs/ast1/extensions.conf 
> PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/channel_test_condition.py 6149 
> 
> Diff: https://reviewboard.asterisk.org/r/4256/diff/
> 
> 
> Testing
> ---
> 
> Currently fails while ASTERISK-24513 is not yet patched.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-- 
_
-- 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] 4256: testsuite: check for channel leak on failed blonde transfer

[Kevin Harwell]

On Jan. 5, 2015, 5 p.m., Scott Griepentrog wrote:
> > Is this missing a sip.conf?
> 
> Scott Griepentrog wrote:
> It's using the bridge test case which is built on sip and includes it's 
> own default sip.conf.  Thus, I don't have to provide one in this test, and it 
> would require changing the bridge test case, or at least creating a new test 
> case - including support directories - so that one could easily switch a test 
> to pjsip without much other changes.  Which is actually not a bad idea, but 
> out of the scope of getting this test in.

Aaah okay yeah that all makes sense then.  No reason to change things up at 
this time.


- Kevin


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


On Dec. 31, 2014, 12:01 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4256/
> ---
> 
> (Updated Dec. 31, 2014, 12:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24513
> https://issues.asterisk.org/jira/browse/ASTERISK-24513
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This test starts an attended transfer, converts to blonde mode by hanging up 
> the transferer, and then fails the transfer by hanging up the transferee.  
> Then after allowing the recall attempt to complete, checks to insure that 
> there are not leaked channels.
> 
> Improvements to channel_test_condition: count the actual channels listed in 
> "core show channels" output to check for leaks.  Also added unittest.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/bridge/tests.yaml 6149 
>   /asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml 
> PRE-CREATION 
>   
> /asterisk/trunk/tests/bridge/atxfer_fail_blonde/configs/ast1/extensions.conf 
> PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/channel_test_condition.py 6149 
> 
> Diff: https://reviewboard.asterisk.org/r/4256/diff/
> 
> 
> Testing
> ---
> 
> Currently fails while ASTERISK-24513 is not yet patched.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-- 
_
-- 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] 4256: testsuite: check for channel leak on failed blonde transfer

[Scott Griepentrog]

On Jan. 5, 2015, 5 p.m., Scott Griepentrog wrote:
> > Is this missing a sip.conf?

It's using the bridge test case which is built on sip and includes it's own 
default sip.conf.  Thus, I don't have to provide one in this test, and it would 
require changing the bridge test case, or at least creating a new test case - 
including support directories - so that one could easily switch a test to pjsip 
without much other changes.  Which is actually not a bad idea, but out of the 
scope of getting this test in.


- Scott


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


On Dec. 31, 2014, 12:01 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4256/
> ---
> 
> (Updated Dec. 31, 2014, 12:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24513
> https://issues.asterisk.org/jira/browse/ASTERISK-24513
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This test starts an attended transfer, converts to blonde mode by hanging up 
> the transferer, and then fails the transfer by hanging up the transferee.  
> Then after allowing the recall attempt to complete, checks to insure that 
> there are not leaked channels.
> 
> Improvements to channel_test_condition: count the actual channels listed in 
> "core show channels" output to check for leaks.  Also added unittest.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/bridge/tests.yaml 6149 
>   /asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml 
> PRE-CREATION 
>   
> /asterisk/trunk/tests/bridge/atxfer_fail_blonde/configs/ast1/extensions.conf 
> PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/channel_test_condition.py 6149 
> 
> Diff: https://reviewboard.asterisk.org/r/4256/diff/
> 
> 
> Testing
> ---
> 
> Currently fails while ASTERISK-24513 is not yet patched.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-- 
_
-- 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