Re: [asterisk-dev] [Code Review] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-12-13 Thread Kevin Harwell

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

(Updated Dec. 13, 2013, 10:40 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


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


Repository: Asterisk


Description
---

Added a channel recording flag that is checked before adding a channel to a 
bridge.  If the the channel is currently being recorded a 409 Conflict is 
returned and the channel is not added to the bridge.


Diffs
-

  branches/12/rest-api/api-docs/bridges.json 402969 
  branches/12/res/stasis/control.c 402969 
  branches/12/res/stasis/command.c 402969 
  branches/12/res/stasis/command.h 402969 
  branches/12/res/res_stasis_recording.c 402969 
  branches/12/res/res_stasis_playback.c 402969 
  branches/12/res/res_stasis_answer.c 402969 
  branches/12/res/res_ari_bridges.c 402969 
  branches/12/res/ari/resource_bridges.c 402969 
  branches/12/include/asterisk/stasis_app_impl.h 402969 
  branches/12/include/asterisk/stasis_app.h 402969 

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


Testing
---

Ran a manual test and also wrote a testsuite test: 
https://reviewboard.asterisk.org/r/2951/


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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-12-12 Thread opticron

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


For the most part, this looks good to go.


branches/12/res/stasis/command.h
https://reviewboard.asterisk.org/r/2947/#comment19797

retval should be enumified.



branches/12/res/stasis/control.c
https://reviewboard.asterisk.org/r/2947/#comment19794

This should be enumified.



branches/12/res/stasis/control.c
https://reviewboard.asterisk.org/r/2947/#comment19795

This should be enumified.



branches/12/res/stasis/control.c
https://reviewboard.asterisk.org/r/2947/#comment19796

This should be enumified.


There are other places as well where functions or parameters should return the 
enum instead of a bare int.

- opticron


On Nov. 21, 2013, 2:24 p.m., Kevin Harwell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/2947/
 ---
 
 (Updated Nov. 21, 2013, 2:24 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: ASTERISK-22624
 https://issues.asterisk.org/jira/browse/ASTERISK-22624
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Added a channel recording flag that is checked before adding a channel to a 
 bridge.  If the the channel is currently being recorded a 409 Conflict is 
 returned and the channel is not added to the bridge.
 
 
 Diffs
 -
 
   branches/12/rest-api/api-docs/bridges.json 402969 
   branches/12/res/stasis/control.c 402969 
   branches/12/res/stasis/command.c 402969 
   branches/12/res/stasis/command.h 402969 
   branches/12/res/res_stasis_recording.c 402969 
   branches/12/res/res_stasis_playback.c 402969 
   branches/12/res/res_stasis_answer.c 402969 
   branches/12/res/res_ari_bridges.c 402969 
   branches/12/res/ari/resource_bridges.c 402969 
   branches/12/include/asterisk/stasis_app_impl.h 402969 
   branches/12/include/asterisk/stasis_app.h 402969 
 
 Diff: https://reviewboard.asterisk.org/r/2947/diff/
 
 
 Testing
 ---
 
 Ran a manual test and also wrote a testsuite test: 
 https://reviewboard.asterisk.org/r/2951/
 
 
 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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-12-12 Thread Kevin Harwell


On Dec. 12, 2013, 9:16 a.m., Kevin Harwell wrote:
  There are other places as well where functions or parameters should return 
  the enum instead of a bare int.

After speaking with opticron it was decided that for now the items mentioned 
should remain ints and not be changed to an enumeration as the may lead to 
undesired specifics in the command code.


- Kevin


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


On Nov. 21, 2013, 2:24 p.m., Kevin Harwell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/2947/
 ---
 
 (Updated Nov. 21, 2013, 2:24 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: ASTERISK-22624
 https://issues.asterisk.org/jira/browse/ASTERISK-22624
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Added a channel recording flag that is checked before adding a channel to a 
 bridge.  If the the channel is currently being recorded a 409 Conflict is 
 returned and the channel is not added to the bridge.
 
 
 Diffs
 -
 
   branches/12/rest-api/api-docs/bridges.json 402969 
   branches/12/res/stasis/control.c 402969 
   branches/12/res/stasis/command.c 402969 
   branches/12/res/stasis/command.h 402969 
   branches/12/res/res_stasis_recording.c 402969 
   branches/12/res/res_stasis_playback.c 402969 
   branches/12/res/res_stasis_answer.c 402969 
   branches/12/res/res_ari_bridges.c 402969 
   branches/12/res/ari/resource_bridges.c 402969 
   branches/12/include/asterisk/stasis_app_impl.h 402969 
   branches/12/include/asterisk/stasis_app.h 402969 
 
 Diff: https://reviewboard.asterisk.org/r/2947/diff/
 
 
 Testing
 ---
 
 Ran a manual test and also wrote a testsuite test: 
 https://reviewboard.asterisk.org/r/2951/
 
 
 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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-12-12 Thread opticron

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

Ship it!


Ship It!

- opticron


On Nov. 21, 2013, 2:24 p.m., Kevin Harwell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/2947/
 ---
 
 (Updated Nov. 21, 2013, 2:24 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: ASTERISK-22624
 https://issues.asterisk.org/jira/browse/ASTERISK-22624
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Added a channel recording flag that is checked before adding a channel to a 
 bridge.  If the the channel is currently being recorded a 409 Conflict is 
 returned and the channel is not added to the bridge.
 
 
 Diffs
 -
 
   branches/12/rest-api/api-docs/bridges.json 402969 
   branches/12/res/stasis/control.c 402969 
   branches/12/res/stasis/command.c 402969 
   branches/12/res/stasis/command.h 402969 
   branches/12/res/res_stasis_recording.c 402969 
   branches/12/res/res_stasis_playback.c 402969 
   branches/12/res/res_stasis_answer.c 402969 
   branches/12/res/res_ari_bridges.c 402969 
   branches/12/res/ari/resource_bridges.c 402969 
   branches/12/include/asterisk/stasis_app_impl.h 402969 
   branches/12/include/asterisk/stasis_app.h 402969 
 
 Diff: https://reviewboard.asterisk.org/r/2947/diff/
 
 
 Testing
 ---
 
 Ran a manual test and also wrote a testsuite test: 
 https://reviewboard.asterisk.org/r/2951/
 
 
 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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-11-21 Thread opticron


 On Nov. 21, 2013, 12:53 p.m., opticron wrote:
  branches/12/include/asterisk/stasis_app.h, line 180
  https://reviewboard.asterisk.org/r/2947/diff/5/?file=48327#file48327line180
 
  This should return the enum instead of int.
 
 Kevin Harwell wrote:
 The problem with making this an enum is currently it is assumed different 
 enum types could be returned.  So instead of having a single master enum for 
 all result types, resources could define and use their expected result types 
 specific to their modules.

If this could return multiple different enum types, collisions between values 
in the enums could produce incorrect error messages. As an example, a rule from 
module B could cause a bridge entry attempted by module A to fail. The enum 
that currently exists is global and not module-specific even though the only 
error value it contains is from a single module.


- opticron


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


On Nov. 21, 2013, 1:50 p.m., Kevin Harwell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/2947/
 ---
 
 (Updated Nov. 21, 2013, 1:50 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: ASTERISK-22624
 https://issues.asterisk.org/jira/browse/ASTERISK-22624
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Added a channel recording flag that is checked before adding a channel to a 
 bridge.  If the the channel is currently being recorded a 409 Conflict is 
 returned and the channel is not added to the bridge.
 
 
 Diffs
 -
 
   branches/12/rest-api/api-docs/bridges.json 402969 
   branches/12/res/stasis/control.c 402969 
   branches/12/res/stasis/command.c 402969 
   branches/12/res/stasis/command.h 402969 
   branches/12/res/res_stasis_recording.c 402969 
   branches/12/res/res_stasis_playback.c 402969 
   branches/12/res/res_stasis_answer.c 402969 
   branches/12/res/res_ari_bridges.c 402969 
   branches/12/res/ari/resource_bridges.c 402969 
   branches/12/include/asterisk/stasis_app_impl.h 402969 
   branches/12/include/asterisk/stasis_app.h 402969 
 
 Diff: https://reviewboard.asterisk.org/r/2947/diff/
 
 
 Testing
 ---
 
 Ran a manual test and also wrote a testsuite test: 
 https://reviewboard.asterisk.org/r/2951/
 
 
 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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-11-21 Thread Kevin Harwell

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

(Updated Nov. 21, 2013, 1:50 p.m.)


Review request for Asterisk Developers.


Changes
---

Update per review items


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


Repository: Asterisk


Description
---

Added a channel recording flag that is checked before adding a channel to a 
bridge.  If the the channel is currently being recorded a 409 Conflict is 
returned and the channel is not added to the bridge.


Diffs (updated)
-

  branches/12/rest-api/api-docs/bridges.json 402969 
  branches/12/res/stasis/control.c 402969 
  branches/12/res/stasis/command.c 402969 
  branches/12/res/stasis/command.h 402969 
  branches/12/res/res_stasis_recording.c 402969 
  branches/12/res/res_stasis_playback.c 402969 
  branches/12/res/res_stasis_answer.c 402969 
  branches/12/res/res_ari_bridges.c 402969 
  branches/12/res/ari/resource_bridges.c 402969 
  branches/12/include/asterisk/stasis_app_impl.h 402969 
  branches/12/include/asterisk/stasis_app.h 402969 

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


Testing
---

Ran a manual test and also wrote a testsuite test: 
https://reviewboard.asterisk.org/r/2951/


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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-11-21 Thread Kevin Harwell

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

(Updated Nov. 21, 2013, 2:24 p.m.)


Review request for Asterisk Developers.


Changes
---

Changed function to return enumeration.


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


Repository: Asterisk


Description
---

Added a channel recording flag that is checked before adding a channel to a 
bridge.  If the the channel is currently being recorded a 409 Conflict is 
returned and the channel is not added to the bridge.


Diffs (updated)
-

  branches/12/rest-api/api-docs/bridges.json 402969 
  branches/12/res/stasis/control.c 402969 
  branches/12/res/stasis/command.c 402969 
  branches/12/res/stasis/command.h 402969 
  branches/12/res/res_stasis_recording.c 402969 
  branches/12/res/res_stasis_playback.c 402969 
  branches/12/res/res_stasis_answer.c 402969 
  branches/12/res/res_ari_bridges.c 402969 
  branches/12/res/ari/resource_bridges.c 402969 
  branches/12/include/asterisk/stasis_app_impl.h 402969 
  branches/12/include/asterisk/stasis_app.h 402969 

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


Testing
---

Ran a manual test and also wrote a testsuite test: 
https://reviewboard.asterisk.org/r/2951/


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] 2947: ARI: Adding a channel to a bridge while a live recording is active blocks

2013-11-21 Thread Kevin Harwell


 On Nov. 21, 2013, 12:53 p.m., opticron wrote:
  branches/12/include/asterisk/stasis_app.h, line 180
  https://reviewboard.asterisk.org/r/2947/diff/5/?file=48327#file48327line180
 
  This should return the enum instead of int.

The problem with making this an enum is currently it is assumed different enum 
types could be returned.  So instead of having a single master enum for all 
result types, resources could define and use their expected result types 
specific to their modules.


- Kevin


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


On Nov. 15, 2013, 1:41 p.m., Kevin Harwell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/2947/
 ---
 
 (Updated Nov. 15, 2013, 1:41 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: ASTERISK-22624
 https://issues.asterisk.org/jira/browse/ASTERISK-22624
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Added a channel recording flag that is checked before adding a channel to a 
 bridge.  If the the channel is currently being recorded a 409 Conflict is 
 returned and the channel is not added to the bridge.
 
 
 Diffs
 -
 
   branches/12/rest-api/api-docs/bridges.json 402853 
   branches/12/res/stasis/control.c 402853 
   branches/12/res/stasis/command.c 402853 
   branches/12/res/stasis/command.h 402853 
   branches/12/res/res_stasis_recording.c 402853 
   branches/12/res/res_stasis_playback.c 402853 
   branches/12/res/res_stasis_answer.c 402853 
   branches/12/res/res_ari_bridges.c 402853 
   branches/12/res/ari/resource_bridges.c 402853 
   branches/12/include/asterisk/stasis_app_impl.h 402853 
   branches/12/include/asterisk/stasis_app.h 402853 
 
 Diff: https://reviewboard.asterisk.org/r/2947/diff/
 
 
 Testing
 ---
 
 Ran a manual test and also wrote a testsuite test: 
 https://reviewboard.asterisk.org/r/2951/
 
 
 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