Re: [asterisk-dev] [Code Review] 3057: CDRs: Synchronize dialplan access through Stasis; clean up Answer and DISA

[Matt Jordan]

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

(Updated Dec. 18, 2013, 6:50 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Bugs: ASTERISK-22884 and ASTERISK-22886
https://issues.asterisk.org/jira/browse/ASTERISK-22884
https://issues.asterisk.org/jira/browse/ASTERISK-22886


Repository: Asterisk


Description
---

When doing the rework of the CDR engine that pushed all of the logic into cdr.c 
and made it respond to changes in channel state over Stasis, I knew that 
accessing the CDR engine from the dialplan would be "slightly" 
non-deterministic. Dialplan threads would be accessing CDRs while Stasis 
threads would be updating the state of said CDRs - whereas in the past, 
everything happened on the dialplan threads. Tests have shown that "slightly" 
is in reality "very".

This patch synchronizes things by making the dialplan applications/functions 
that manipulate CDRs do so over Stasis. ForkCDR, NoCDR, ResetCDR, CDR, and 
CDR_PROP now all use Stasis to send their requests over to the CDR engine, and 
synchronize on the Stasis subscription so that they return their values/control 
to the dialplan at the appropriate time.

While going through this, the following changes were also made:
 * DISA, which can reset the CDR when a user successfully authenticates, now 
just uses the ResetCDR app to do this. This prevents having to duplicate the 
same Stasis synchronization logic in that application.
 * Answer no longer disables CDRs. It actually didn't work anyway - calling 
DISABLE on the channel's CDR doesn't stop the CDR from getting the Answer time 
- it just kills all CDRs on that channel, which isn't what the caller would 
intend.


Diffs
-

  /branches/12/main/pbx.c 403703 
  /branches/12/main/cdr.c 403703 
  /branches/12/include/asterisk/cdr.h 403703 
  /branches/12/funcs/func_cdr.c 403703 
  /branches/12/apps/app_forkcdr.c 403703 
  /branches/12/apps/app_disa.c 403703 
  /branches/12/apps/app_cdr.c 403703 
  /branches/12/UPGRADE.txt 403703 
  /branches/12/CHANGES 403703 

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


Testing
---

* The CDR tests pass (now deterministicly)
* The hangup handler tests now pass, as they can get reliable values in the h 
extension from the CDR engine


Thanks,

Matt Jordan

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

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

Re: [asterisk-dev] [Code Review] 3079: udptl: Dead code elimination. ast_udptl_bridge not used.

[rmudgett]

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

(Updated Dec. 19, 2013, 12:21 a.m.)


Review request for Asterisk Developers.


Repository: Asterisk


Description (updated)
---

Removing dead code starting with ast_udptl_bridge() eliminated the code in this 
patch.

Note: This code has actually been dead since at least Asterisk v1.8.


Diffs
-

  /branches/12/main/udptl.c 404291 
  /branches/12/include/asterisk/udptl.h 404291 
  /branches/12/channels/chan_sip.c 404291 
  /branches/12/addons/chan_ooh323.c 404291 

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


Testing
---

It still compiles because nothing used what was deleted. :)


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] 3079: udptl: Dead code elimination. ast_udptl_bridge not used.

[rmudgett]

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

Review request for Asterisk Developers.


Repository: Asterisk


Description
---

Removing dead code starting with ast_udptl_bridge() eliminated the code in this 
patch.


Diffs
-

  /branches/12/main/udptl.c 404291 
  /branches/12/include/asterisk/udptl.h 404291 
  /branches/12/channels/chan_sip.c 404291 
  /branches/12/addons/chan_ooh323.c 404291 

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


Testing
---

It still compiles because nothing used what was deleted. :)


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] 3069: Fix deadlock experienced during multi-party PJSIP transfer through masquerade rework

[rmudgett]

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

Ship it!


Ship It!

- rmudgett


On Dec. 18, 2013, 11:10 p.m., Mark Michelson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3069/
> ---
> 
> (Updated Dec. 18, 2013, 11:10 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Jonathan Rose, Matt 
> Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-22936
> https://issues.asterisk.org/jira/browse/ASTERISK-22936
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This is more-or-less a take 2 of https://reviewboard.asterisk.org/r/3042
> 
> Initially, it was discovered that performing an attended transfer of a 
> multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread 
> started a masquerade and reached the point where it was calling the fixup() 
> callback on the "original" channel. For chan_pjsip, this involves pushing a 
> synchronous task to the session's serializer. The problem was that a task 
> ahead of the fixup task was also attempting to perform a channel masquerade. 
> However, since masquerades are designed in a way to only allow for one to 
> occur at a time, the task ahead of the fixup could not continue until the 
> masquerade already in progress had completed. And of course, the masquerade 
> in progress could not complete until the task ahead of the fixup task had 
> completed. Deadlock.
> 
> The initial fix was to change the fixup task to be asynchronous. While this 
> prevented the deadlock from occurring, it had the frightful side effect of 
> potentially allowing for tasks in the session's serializer to operate on a 
> zombie channel.
> 
> Taking a step back from this particular deadlock, it became clear that the 
> problem was not really this one particular issue but that masquerades 
> themselves needed to be addressed. A PJSIP attended transfer operation calls 
> ast_channel_move(), which attempts to both set up and execute a masquerade. 
> The problem was that after it had set up the masquerade, the PBX thread had 
> swooped in and tried to actually perform the masquerade. Looking at changes 
> that had been made to Asterisk 12, it became clear that there never is any 
> time now that anyone ever wants to set up a masquerade and allow for the 
> channel thread to actually perform the masquerade. Everyone always is calling 
> ast_channel_move(), performs the masquerade itself before returning.
> 
> In this patch, I have removed all blocks of code from channel.c that will 
> attempt to perform a masquerade if ast_channel_masq() returns true. Now, 
> there is no distinction between setting up a masquerade and performing the 
> masquerade. It is one operation. The only remaining checks for 
> ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do 
> not want to interrupt a masquerade by hanging up the channel. Instead, now 
> ast_hangup() will wait for a masquerade to complete before moving forward 
> with its operation.
> 
> The ast_channel_move() function has been modified to basically in-line the 
> logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() 
> has been killed off for real. ast_channel_move() now has a lock associated 
> with it that is used to prevent any simultaneous moves from occurring at 
> once. This means there is no need to make sure that ast_channel_masq() or 
> ast_channel_masqr() are already set on a channel when ast_channel_move() is 
> called. It also means the channel container lock is not pulling double duty 
> by both keeping the container locked and preventing multiple masquerades from 
> occurring simultaneously.
> 
> The ast_do_masquerade() function has been renamed to do_channel_masquerade() 
> and is now internal to channel.c. The function now takes explicit arguments 
> of which channels are involved in the masquerade instead of a single channel. 
> While it probably is possible to do some further refactoring of this method, 
> I feel that I would be treading dangerously. Instead, all I did was change 
> some comments that no longer are true after this changeset.
> 
> The other more minor change introduced in this patch is to res_pjsip.c to 
> make ast_sip_push_task_synchronous() run the task in-place if we are already 
> a SIP servant thread. This is related to this patch because even when we 
> isolate the channel masquerade to only running in the SIP servant thread, we 
> would still deadlock when the fixup() callback is reached since we would 
> essentially be waiting forever for ourselves to finish before actually 
> running the fixup. This makes it so the fixup is run without having to push a 
> task into 

Re: [asterisk-dev] [Code Review] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[Mark Michelson]

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

Ship it!


Ship It!

- Mark Michelson


On Dec. 18, 2013, 11:40 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3072/
> ---
> 
> (Updated Dec. 18, 2013, 11:40 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Removed code from the system for normal mailbox handling that appends 
> @default to the mailbox identifier if it does not have a context.  The only 
> exception is the legacy hasvoicemail users.conf option.  The legacy option 
> will only work for app_voicemail mailboxes.  (I'd like to just remove the 
> hasvoicemail option since it is usually superceeded by an explicit mailbox 
> option. :) )  The system cannot make any assumptions about the format of the 
> mailbox identifer used by app_voicemail.
> 
> chan_sip and chan_dahdi/sig_pri had the most changes because they both tried 
> to interpret the mailbox identifier.  chan_sip just stored and compared the 
> two components.  chan_dahdi actually used the box information.
> 
> The ISDN MWI support configuration options had to be reworked because 
> chan_dahdi was parsing the box@context format to get the box number.  As a 
> result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
> the chan_dahdi.conf.sample file.
> 
> 
> Diffs
> -
> 
>   /branches/12/res/res_xmpp.c 404264 
>   /branches/12/res/res_jabber.c 404264 
>   /branches/12/main/app.c 404264 
>   /branches/12/include/asterisk/app.h 404264 
>   /branches/12/funcs/func_vmcount.c 404264 
>   /branches/12/configs/voicemail.conf.sample 404264 
>   /branches/12/configs/skinny.conf.sample 404264 
>   /branches/12/configs/sip.conf.sample 404264 
>   /branches/12/configs/iax.conf.sample 404264 
>   /branches/12/configs/chan_dahdi.conf.sample 404264 
>   /branches/12/channels/sip/include/sip.h 404264 
>   /branches/12/channels/sig_pri.c 404264 
>   /branches/12/channels/sig_pri.h 404264 
>   /branches/12/channels/h323/chan_h323.h 404264 
>   /branches/12/channels/chan_unistim.c 404264 
>   /branches/12/channels/chan_skinny.c 404264 
>   /branches/12/channels/chan_sip.c 404264 
>   /branches/12/channels/chan_mgcp.c 404264 
>   /branches/12/channels/chan_iax2.c 404264 
>   /branches/12/channels/chan_h323.c 404264 
>   /branches/12/channels/chan_dahdi.c 404264 
>   /branches/12/channels/chan_dahdi.h 404264 
>   /branches/12/apps/app_voicemail.c 404264 
>   /branches/12/UPGRADE.txt 404264 
>   /branches/12/CHANGES 404264 
> 
> Diff: https://reviewboard.asterisk.org/r/3072/diff/
> 
> 
> Testing
> ---
> 
> Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses 
> correctly and sets up mailboxes.
> Tested chan_sip to be sure that the mailbox option works.
> 
> 
> 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] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[rmudgett]

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

(Updated Dec. 18, 2013, 11:40 p.m.)


Review request for Asterisk Developers.


Changes
---

Address more feedback.


Repository: Asterisk


Description
---

Removed code from the system for normal mailbox handling that appends @default 
to the mailbox identifier if it does not have a context.  The only exception is 
the legacy hasvoicemail users.conf option.  The legacy option will only work 
for app_voicemail mailboxes.  (I'd like to just remove the hasvoicemail option 
since it is usually superceeded by an explicit mailbox option. :) )  The system 
cannot make any assumptions about the format of the mailbox identifer used by 
app_voicemail.

chan_sip and chan_dahdi/sig_pri had the most changes because they both tried to 
interpret the mailbox identifier.  chan_sip just stored and compared the two 
components.  chan_dahdi actually used the box information.

The ISDN MWI support configuration options had to be reworked because 
chan_dahdi was parsing the box@context format to get the box number.  As a 
result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
the chan_dahdi.conf.sample file.


Diffs (updated)
-

  /branches/12/res/res_xmpp.c 404264 
  /branches/12/res/res_jabber.c 404264 
  /branches/12/main/app.c 404264 
  /branches/12/include/asterisk/app.h 404264 
  /branches/12/funcs/func_vmcount.c 404264 
  /branches/12/configs/voicemail.conf.sample 404264 
  /branches/12/configs/skinny.conf.sample 404264 
  /branches/12/configs/sip.conf.sample 404264 
  /branches/12/configs/iax.conf.sample 404264 
  /branches/12/configs/chan_dahdi.conf.sample 404264 
  /branches/12/channels/sip/include/sip.h 404264 
  /branches/12/channels/sig_pri.c 404264 
  /branches/12/channels/sig_pri.h 404264 
  /branches/12/channels/h323/chan_h323.h 404264 
  /branches/12/channels/chan_unistim.c 404264 
  /branches/12/channels/chan_skinny.c 404264 
  /branches/12/channels/chan_sip.c 404264 
  /branches/12/channels/chan_mgcp.c 404264 
  /branches/12/channels/chan_iax2.c 404264 
  /branches/12/channels/chan_h323.c 404264 
  /branches/12/channels/chan_dahdi.c 404264 
  /branches/12/channels/chan_dahdi.h 404264 
  /branches/12/apps/app_voicemail.c 404264 
  /branches/12/UPGRADE.txt 404264 
  /branches/12/CHANGES 404264 

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


Testing
---

Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses correctly 
and sets up mailboxes.
Tested chan_sip to be sure that the mailbox option works.


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] 3069: Fix deadlock experienced during multi-party PJSIP transfer through masquerade rework

[Mark Michelson]

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

(Updated Dec. 18, 2013, 11:10 p.m.)


Review request for Asterisk Developers, Joshua Colp, Jonathan Rose, Matt 
Jordan, and rmudgett.


Changes
---

Addressed latest feedback.


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


Repository: Asterisk


Description
---

This is more-or-less a take 2 of https://reviewboard.asterisk.org/r/3042

Initially, it was discovered that performing an attended transfer of a 
multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread 
started a masquerade and reached the point where it was calling the fixup() 
callback on the "original" channel. For chan_pjsip, this involves pushing a 
synchronous task to the session's serializer. The problem was that a task ahead 
of the fixup task was also attempting to perform a channel masquerade. However, 
since masquerades are designed in a way to only allow for one to occur at a 
time, the task ahead of the fixup could not continue until the masquerade 
already in progress had completed. And of course, the masquerade in progress 
could not complete until the task ahead of the fixup task had completed. 
Deadlock.

The initial fix was to change the fixup task to be asynchronous. While this 
prevented the deadlock from occurring, it had the frightful side effect of 
potentially allowing for tasks in the session's serializer to operate on a 
zombie channel.

Taking a step back from this particular deadlock, it became clear that the 
problem was not really this one particular issue but that masquerades 
themselves needed to be addressed. A PJSIP attended transfer operation calls 
ast_channel_move(), which attempts to both set up and execute a masquerade. The 
problem was that after it had set up the masquerade, the PBX thread had swooped 
in and tried to actually perform the masquerade. Looking at changes that had 
been made to Asterisk 12, it became clear that there never is any time now that 
anyone ever wants to set up a masquerade and allow for the channel thread to 
actually perform the masquerade. Everyone always is calling ast_channel_move(), 
performs the masquerade itself before returning.

In this patch, I have removed all blocks of code from channel.c that will 
attempt to perform a masquerade if ast_channel_masq() returns true. Now, there 
is no distinction between setting up a masquerade and performing the 
masquerade. It is one operation. The only remaining checks for 
ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do not 
want to interrupt a masquerade by hanging up the channel. Instead, now 
ast_hangup() will wait for a masquerade to complete before moving forward with 
its operation.

The ast_channel_move() function has been modified to basically in-line the 
logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() has 
been killed off for real. ast_channel_move() now has a lock associated with it 
that is used to prevent any simultaneous moves from occurring at once. This 
means there is no need to make sure that ast_channel_masq() or 
ast_channel_masqr() are already set on a channel when ast_channel_move() is 
called. It also means the channel container lock is not pulling double duty by 
both keeping the container locked and preventing multiple masquerades from 
occurring simultaneously.

The ast_do_masquerade() function has been renamed to do_channel_masquerade() 
and is now internal to channel.c. The function now takes explicit arguments of 
which channels are involved in the masquerade instead of a single channel. 
While it probably is possible to do some further refactoring of this method, I 
feel that I would be treading dangerously. Instead, all I did was change some 
comments that no longer are true after this changeset.

The other more minor change introduced in this patch is to res_pjsip.c to make 
ast_sip_push_task_synchronous() run the task in-place if we are already a SIP 
servant thread. This is related to this patch because even when we isolate the 
channel masquerade to only running in the SIP servant thread, we would still 
deadlock when the fixup() callback is reached since we would essentially be 
waiting forever for ourselves to finish before actually running the fixup. This 
makes it so the fixup is run without having to push a task into a serializer at 
all.


Diffs (updated)
-

  /branches/12/res/res_pjsip.c 403778 
  /branches/12/main/channel.c 403778 
  /branches/12/include/asterisk/channel.h 403778 
  /branches/12/include/asterisk/autochan.h 403778 

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


Testing
---

Jonathan ran the same test he had originally run in order to make the deadlock 
occur, and with this patch it no longer occurs. I also ran a bevy of test

Re: [asterisk-dev] [Code Review] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[Mark Michelson]

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



/branches/12/apps/app_voicemail.c


Going through this code, I've now seen this repeated three times in 
app_voicemail. Place this into its own function.

/* Prototype */
int separate_mailbox(char *mailbox_id, char **context, char **mailbox);

/* Usage */
if (separate_mailbox(ast_strdupa(mailbox_id), &context, &mailbox)) {
return 0;
}



/branches/12/configs/chan_dahdi.conf.sample


This could use a mention in UPGRADE.txt since previously the command did 
not specify a context.


- Mark Michelson


On Dec. 18, 2013, 6:51 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3072/
> ---
> 
> (Updated Dec. 18, 2013, 6:51 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Removed code from the system for normal mailbox handling that appends 
> @default to the mailbox identifier if it does not have a context.  The only 
> exception is the legacy hasvoicemail users.conf option.  The legacy option 
> will only work for app_voicemail mailboxes.  (I'd like to just remove the 
> hasvoicemail option since it is usually superceeded by an explicit mailbox 
> option. :) )  The system cannot make any assumptions about the format of the 
> mailbox identifer used by app_voicemail.
> 
> chan_sip and chan_dahdi/sig_pri had the most changes because they both tried 
> to interpret the mailbox identifier.  chan_sip just stored and compared the 
> two components.  chan_dahdi actually used the box information.
> 
> The ISDN MWI support configuration options had to be reworked because 
> chan_dahdi was parsing the box@context format to get the box number.  As a 
> result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
> the chan_dahdi.conf.sample file.
> 
> 
> Diffs
> -
> 
>   /branches/12/res/res_xmpp.c 404197 
>   /branches/12/res/res_jabber.c 404197 
>   /branches/12/main/app.c 404197 
>   /branches/12/include/asterisk/app.h 404197 
>   /branches/12/funcs/func_vmcount.c 404197 
>   /branches/12/configs/voicemail.conf.sample 404197 
>   /branches/12/configs/skinny.conf.sample 404197 
>   /branches/12/configs/sip.conf.sample 404197 
>   /branches/12/configs/iax.conf.sample 404197 
>   /branches/12/configs/chan_dahdi.conf.sample 404197 
>   /branches/12/channels/sip/include/sip.h 404197 
>   /branches/12/channels/sig_pri.c 404197 
>   /branches/12/channels/sig_pri.h 404197 
>   /branches/12/channels/h323/chan_h323.h 404197 
>   /branches/12/channels/chan_unistim.c 404197 
>   /branches/12/channels/chan_skinny.c 404197 
>   /branches/12/channels/chan_sip.c 404197 
>   /branches/12/channels/chan_mgcp.c 404197 
>   /branches/12/channels/chan_iax2.c 404197 
>   /branches/12/channels/chan_h323.c 404197 
>   /branches/12/channels/chan_dahdi.c 404197 
>   /branches/12/channels/chan_dahdi.h 404197 
>   /branches/12/apps/app_voicemail.c 404197 
>   /branches/12/UPGRADE.txt 404197 
>   /branches/12/CHANGES 404197 
> 
> Diff: https://reviewboard.asterisk.org/r/3072/diff/
> 
> 
> Testing
> ---
> 
> Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses 
> correctly and sets up mailboxes.
> Tested chan_sip to be sure that the mailbox option works.
> 
> 
> 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] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[Matt Jordan]

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

Ship it!



/branches/12/CHANGES


For formatting consistency, add a line of '' below VMCOUNT.


- Matt Jordan


On Dec. 18, 2013, 12:51 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3072/
> ---
> 
> (Updated Dec. 18, 2013, 12:51 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Removed code from the system for normal mailbox handling that appends 
> @default to the mailbox identifier if it does not have a context.  The only 
> exception is the legacy hasvoicemail users.conf option.  The legacy option 
> will only work for app_voicemail mailboxes.  (I'd like to just remove the 
> hasvoicemail option since it is usually superceeded by an explicit mailbox 
> option. :) )  The system cannot make any assumptions about the format of the 
> mailbox identifer used by app_voicemail.
> 
> chan_sip and chan_dahdi/sig_pri had the most changes because they both tried 
> to interpret the mailbox identifier.  chan_sip just stored and compared the 
> two components.  chan_dahdi actually used the box information.
> 
> The ISDN MWI support configuration options had to be reworked because 
> chan_dahdi was parsing the box@context format to get the box number.  As a 
> result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
> the chan_dahdi.conf.sample file.
> 
> 
> Diffs
> -
> 
>   /branches/12/res/res_xmpp.c 404197 
>   /branches/12/res/res_jabber.c 404197 
>   /branches/12/main/app.c 404197 
>   /branches/12/include/asterisk/app.h 404197 
>   /branches/12/funcs/func_vmcount.c 404197 
>   /branches/12/configs/voicemail.conf.sample 404197 
>   /branches/12/configs/skinny.conf.sample 404197 
>   /branches/12/configs/sip.conf.sample 404197 
>   /branches/12/configs/iax.conf.sample 404197 
>   /branches/12/configs/chan_dahdi.conf.sample 404197 
>   /branches/12/channels/sip/include/sip.h 404197 
>   /branches/12/channels/sig_pri.c 404197 
>   /branches/12/channels/sig_pri.h 404197 
>   /branches/12/channels/h323/chan_h323.h 404197 
>   /branches/12/channels/chan_unistim.c 404197 
>   /branches/12/channels/chan_skinny.c 404197 
>   /branches/12/channels/chan_sip.c 404197 
>   /branches/12/channels/chan_mgcp.c 404197 
>   /branches/12/channels/chan_iax2.c 404197 
>   /branches/12/channels/chan_h323.c 404197 
>   /branches/12/channels/chan_dahdi.c 404197 
>   /branches/12/channels/chan_dahdi.h 404197 
>   /branches/12/apps/app_voicemail.c 404197 
>   /branches/12/UPGRADE.txt 404197 
>   /branches/12/CHANGES 404197 
> 
> Diff: https://reviewboard.asterisk.org/r/3072/diff/
> 
> 
> Testing
> ---
> 
> Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses 
> correctly and sets up mailboxes.
> Tested chan_sip to be sure that the mailbox option works.
> 
> 
> 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] 3069: Fix deadlock experienced during multi-party PJSIP transfer through masquerade rework

[rmudgett]

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



/branches/12/main/channel.c


This is mostly still true.  Change to:

The masq and masqr pointers need to be left alone until the masquerade has 
restabilized the channels to hold off ast_hangup() and until the 
AST_FLAG_ZOMBIE can be set on the clonechan.




/branches/12/main/channel.c


I think the masq and masqr pointers need to be cleared later at the same 
time where you are clearing the masq pointer now.  They also need to be cleared 
with both channels locked.  This way agent.c and features.c could get the 
masq/masqr channel name safely if they had the channel locked.



/branches/12/main/channel.c


This comment can be removed.  Delaying the masq/masqr clearing holds off 
the hangup and another masquerade cannot be started until this one finished.



/branches/12/main/channel.c


Lock both channels and clear masq/masqr here.


- rmudgett


On Dec. 18, 2013, 8:20 p.m., Mark Michelson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3069/
> ---
> 
> (Updated Dec. 18, 2013, 8:20 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Jonathan Rose, Matt 
> Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-22936
> https://issues.asterisk.org/jira/browse/ASTERISK-22936
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This is more-or-less a take 2 of https://reviewboard.asterisk.org/r/3042
> 
> Initially, it was discovered that performing an attended transfer of a 
> multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread 
> started a masquerade and reached the point where it was calling the fixup() 
> callback on the "original" channel. For chan_pjsip, this involves pushing a 
> synchronous task to the session's serializer. The problem was that a task 
> ahead of the fixup task was also attempting to perform a channel masquerade. 
> However, since masquerades are designed in a way to only allow for one to 
> occur at a time, the task ahead of the fixup could not continue until the 
> masquerade already in progress had completed. And of course, the masquerade 
> in progress could not complete until the task ahead of the fixup task had 
> completed. Deadlock.
> 
> The initial fix was to change the fixup task to be asynchronous. While this 
> prevented the deadlock from occurring, it had the frightful side effect of 
> potentially allowing for tasks in the session's serializer to operate on a 
> zombie channel.
> 
> Taking a step back from this particular deadlock, it became clear that the 
> problem was not really this one particular issue but that masquerades 
> themselves needed to be addressed. A PJSIP attended transfer operation calls 
> ast_channel_move(), which attempts to both set up and execute a masquerade. 
> The problem was that after it had set up the masquerade, the PBX thread had 
> swooped in and tried to actually perform the masquerade. Looking at changes 
> that had been made to Asterisk 12, it became clear that there never is any 
> time now that anyone ever wants to set up a masquerade and allow for the 
> channel thread to actually perform the masquerade. Everyone always is calling 
> ast_channel_move(), performs the masquerade itself before returning.
> 
> In this patch, I have removed all blocks of code from channel.c that will 
> attempt to perform a masquerade if ast_channel_masq() returns true. Now, 
> there is no distinction between setting up a masquerade and performing the 
> masquerade. It is one operation. The only remaining checks for 
> ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do 
> not want to interrupt a masquerade by hanging up the channel. Instead, now 
> ast_hangup() will wait for a masquerade to complete before moving forward 
> with its operation.
> 
> The ast_channel_move() function has been modified to basically in-line the 
> logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() 
> has been killed off for real. ast_channel_move() now has a lock associated 
> with it that is used to prevent any simultaneous moves from occurring at 
> once. This means there is no need to make sure that ast_channel_masq() or 
> ast_channel_masqr() are already set on a channel when ast_channel_move() is 
> called. It also means the channel container lock is not pulling double duty 
> by both keeping the container locked and preventing m

Re: [asterisk-dev] [Code Review] 3078: astdb: crash in sqlite3 during shutdown

[Mark Michelson]

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

Ship it!


Ship It!

- Mark Michelson


On Dec. 18, 2013, 9:26 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3078/
> ---
> 
> (Updated Dec. 18, 2013, 9:26 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1265 and ASTERISK-22350
> https://issues.asterisk.org/jira/browse/AST-1265
> https://issues.asterisk.org/jira/browse/ASTERISK-22350
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When Asterisk is shut down, the astdb_atexit() function releases (finalizes) 
> the previously initialized (prepared) SQL statements in sqlite3.  Another 
> thread subsequently attempting an astdb operation tries to use the released 
> statement and causes a violation in sqlite3.  This patch eliminates that 
> issue by resetting the statement pointer after it is released/cleared.  Code 
> in sqlite3 will detect the null pointer and abort the operation rather than 
> crash.
> 
> 
> Diffs
> -
> 
>   /branches/11/main/db.c 404197 
> 
> Diff: https://reviewboard.asterisk.org/r/3078/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 3078: astdb: crash in sqlite3 during shutdown

[Scott Griepentrog]

> On Dec. 18, 2013, 2:53 p.m., Mark Michelson wrote:
> > /branches/11/main/db.c, lines 150-156
> > 
> >
> > I got a bit curious about how sqlite3_finalize() works, so I took a 
> > look at the source code.
> > 
> > If I'm reading it correctly, even if an error is returned, the prepared 
> > statement is actually freed. The return simply indicates whether the last 
> > execution of the statement was successful or not.
> > 
> > Having said that, I believe that you need to NULL out *stmt whether 
> > sqlite3_finalize() returns an error or not.

Agreed.


- Scott


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


On Dec. 18, 2013, 1:42 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3078/
> ---
> 
> (Updated Dec. 18, 2013, 1:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1265 and ASTERISK-22350
> https://issues.asterisk.org/jira/browse/AST-1265
> https://issues.asterisk.org/jira/browse/ASTERISK-22350
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When Asterisk is shut down, the astdb_atexit() function releases (finalizes) 
> the previously initialized (prepared) SQL statements in sqlite3.  Another 
> thread subsequently attempting an astdb operation tries to use the released 
> statement and causes a violation in sqlite3.  This patch eliminates that 
> issue by resetting the statement pointer after it is released/cleared.  Code 
> in sqlite3 will detect the null pointer and abort the operation rather than 
> crash.
> 
> 
> Diffs
> -
> 
>   /branches/11/main/db.c 404197 
> 
> Diff: https://reviewboard.asterisk.org/r/3078/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 3078: astdb: crash in sqlite3 during shutdown

[Scott Griepentrog]

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

(Updated Dec. 18, 2013, 3:26 p.m.)


Review request for Asterisk Developers.


Changes
---

statement is now null'd even on error 


Bugs: AST-1265 and ASTERISK-22350
https://issues.asterisk.org/jira/browse/AST-1265
https://issues.asterisk.org/jira/browse/ASTERISK-22350


Repository: Asterisk


Description
---

When Asterisk is shut down, the astdb_atexit() function releases (finalizes) 
the previously initialized (prepared) SQL statements in sqlite3.  Another 
thread subsequently attempting an astdb operation tries to use the released 
statement and causes a violation in sqlite3.  This patch eliminates that issue 
by resetting the statement pointer after it is released/cleared.  Code in 
sqlite3 will detect the null pointer and abort the operation rather than crash.


Diffs (updated)
-

  /branches/11/main/db.c 404197 

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


Testing
---


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] 3078: astdb: crash in sqlite3 during shutdown

[Mark Michelson]

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



/branches/11/main/db.c


I got a bit curious about how sqlite3_finalize() works, so I took a look at 
the source code.

If I'm reading it correctly, even if an error is returned, the prepared 
statement is actually freed. The return simply indicates whether the last 
execution of the statement was successful or not.

Having said that, I believe that you need to NULL out *stmt whether 
sqlite3_finalize() returns an error or not.


- Mark Michelson


On Dec. 18, 2013, 7:42 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3078/
> ---
> 
> (Updated Dec. 18, 2013, 7:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1265 and ASTERISK-22350
> https://issues.asterisk.org/jira/browse/AST-1265
> https://issues.asterisk.org/jira/browse/ASTERISK-22350
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When Asterisk is shut down, the astdb_atexit() function releases (finalizes) 
> the previously initialized (prepared) SQL statements in sqlite3.  Another 
> thread subsequently attempting an astdb operation tries to use the released 
> statement and causes a violation in sqlite3.  This patch eliminates that 
> issue by resetting the statement pointer after it is released/cleared.  Code 
> in sqlite3 will detect the null pointer and abort the operation rather than 
> crash.
> 
> 
> Diffs
> -
> 
>   /branches/11/main/db.c 404197 
> 
> Diff: https://reviewboard.asterisk.org/r/3078/diff/
> 
> 
> Testing
> ---
> 
> 
> 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] 3069: Fix deadlock experienced during multi-party PJSIP transfer through masquerade rework

[Mark Michelson]

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

(Updated Dec. 18, 2013, 8:20 p.m.)


Review request for Asterisk Developers, Joshua Colp, Jonathan Rose, Matt 
Jordan, and rmudgett.


Changes
---

Addressed review feedback.


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


Repository: Asterisk


Description
---

This is more-or-less a take 2 of https://reviewboard.asterisk.org/r/3042

Initially, it was discovered that performing an attended transfer of a 
multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread 
started a masquerade and reached the point where it was calling the fixup() 
callback on the "original" channel. For chan_pjsip, this involves pushing a 
synchronous task to the session's serializer. The problem was that a task ahead 
of the fixup task was also attempting to perform a channel masquerade. However, 
since masquerades are designed in a way to only allow for one to occur at a 
time, the task ahead of the fixup could not continue until the masquerade 
already in progress had completed. And of course, the masquerade in progress 
could not complete until the task ahead of the fixup task had completed. 
Deadlock.

The initial fix was to change the fixup task to be asynchronous. While this 
prevented the deadlock from occurring, it had the frightful side effect of 
potentially allowing for tasks in the session's serializer to operate on a 
zombie channel.

Taking a step back from this particular deadlock, it became clear that the 
problem was not really this one particular issue but that masquerades 
themselves needed to be addressed. A PJSIP attended transfer operation calls 
ast_channel_move(), which attempts to both set up and execute a masquerade. The 
problem was that after it had set up the masquerade, the PBX thread had swooped 
in and tried to actually perform the masquerade. Looking at changes that had 
been made to Asterisk 12, it became clear that there never is any time now that 
anyone ever wants to set up a masquerade and allow for the channel thread to 
actually perform the masquerade. Everyone always is calling ast_channel_move(), 
performs the masquerade itself before returning.

In this patch, I have removed all blocks of code from channel.c that will 
attempt to perform a masquerade if ast_channel_masq() returns true. Now, there 
is no distinction between setting up a masquerade and performing the 
masquerade. It is one operation. The only remaining checks for 
ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do not 
want to interrupt a masquerade by hanging up the channel. Instead, now 
ast_hangup() will wait for a masquerade to complete before moving forward with 
its operation.

The ast_channel_move() function has been modified to basically in-line the 
logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() has 
been killed off for real. ast_channel_move() now has a lock associated with it 
that is used to prevent any simultaneous moves from occurring at once. This 
means there is no need to make sure that ast_channel_masq() or 
ast_channel_masqr() are already set on a channel when ast_channel_move() is 
called. It also means the channel container lock is not pulling double duty by 
both keeping the container locked and preventing multiple masquerades from 
occurring simultaneously.

The ast_do_masquerade() function has been renamed to do_channel_masquerade() 
and is now internal to channel.c. The function now takes explicit arguments of 
which channels are involved in the masquerade instead of a single channel. 
While it probably is possible to do some further refactoring of this method, I 
feel that I would be treading dangerously. Instead, all I did was change some 
comments that no longer are true after this changeset.

The other more minor change introduced in this patch is to res_pjsip.c to make 
ast_sip_push_task_synchronous() run the task in-place if we are already a SIP 
servant thread. This is related to this patch because even when we isolate the 
channel masquerade to only running in the SIP servant thread, we would still 
deadlock when the fixup() callback is reached since we would essentially be 
waiting forever for ourselves to finish before actually running the fixup. This 
makes it so the fixup is run without having to push a task into a serializer at 
all.


Diffs (updated)
-

  /branches/12/res/res_pjsip.c 403778 
  /branches/12/main/channel.c 403778 
  /branches/12/include/asterisk/channel.h 403778 
  /branches/12/include/asterisk/autochan.h 403778 

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


Testing
---

Jonathan ran the same test he had originally run in order to make the deadlock 
occur, and with this patch it no longer occurs. I also ran a bevy of tests

[asterisk-dev] [Code Review] 3078: astdb: crash in sqlite3 during shutdown

[Scott Griepentrog]

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

Review request for Asterisk Developers.


Bugs: AST-1265 and ASTERISK-22350
https://issues.asterisk.org/jira/browse/AST-1265
https://issues.asterisk.org/jira/browse/ASTERISK-22350


Repository: Asterisk


Description
---

When Asterisk is shut down, the astdb_atexit() function releases (finalizes) 
the previously initialized (prepared) SQL statements in sqlite3.  Another 
thread subsequently attempting an astdb operation tries to use the released 
statement and causes a violation in sqlite3.  This patch eliminates that issue 
by resetting the statement pointer after it is released/cleared.  Code in 
sqlite3 will detect the null pointer and abort the operation rather than crash.


Diffs
-

  /branches/11/main/db.c 404197 

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


Testing
---


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] 3067: channels: Return channel locked when allocating.

[Joshua Colp]

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

(Updated Dec. 18, 2013, 7:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Repository: Asterisk


Description
---

Currently when allocating a channel it is possible for external entities to 
become aware of the channel and try to interact with it before it is completely 
set up. This occurs because the channel is returned unlocked, and the caller 
allocating it is then expected to lock, populate it, and unlock. This leaves a 
small window where something else can get the channel.

The attached change makes it so ast_channel_alloc returns the allocated channel 
locked. It is expected that the caller then populates and unlocks. This leaves 
no window for something external to get and use it.


Diffs
-

  /branches/12/tests/test_voicemail_api.c 404134 
  /branches/12/tests/test_substitution.c 404134 
  /branches/12/tests/test_stasis_channels.c 404134 
  /branches/12/tests/test_cel.c 404134 
  /branches/12/tests/test_cdr.c 404134 
  /branches/12/tests/test_app.c 404134 
  /branches/12/res/res_stasis_snoop.c 404134 
  /branches/12/res/res_calendar.c 404137 
  /branches/12/res/parking/parking_tests.c 404134 
  /branches/12/main/pbx.c 404134 
  /branches/12/main/message.c 404134 
  /branches/12/main/core_unreal.c 404134 
  /branches/12/main/channel.c 404134 
  /branches/12/include/asterisk/channel.h 404134 
  /branches/12/channels/chan_vpb.cc 404134 
  /branches/12/channels/chan_unistim.c 404134 
  /branches/12/channels/chan_skinny.c 404134 
  /branches/12/channels/chan_sip.c 404134 
  /branches/12/channels/chan_pjsip.c 404134 
  /branches/12/channels/chan_phone.c 404134 
  /branches/12/channels/chan_oss.c 404134 
  /branches/12/channels/chan_nbs.c 404134 
  /branches/12/channels/chan_multicast_rtp.c 404134 
  /branches/12/channels/chan_motif.c 404134 
  /branches/12/channels/chan_misdn.c 404134 
  /branches/12/channels/chan_mgcp.c 404134 
  /branches/12/channels/chan_jingle.c 404134 
  /branches/12/channels/chan_iax2.c 404134 
  /branches/12/channels/chan_h323.c 404134 
  /branches/12/channels/chan_gtalk.c 404134 
  /branches/12/channels/chan_dahdi.c 404134 
  /branches/12/channels/chan_console.c 404134 
  /branches/12/channels/chan_alsa.c 404134 
  /branches/12/apps/confbridge/conf_chan_record.c 404134 
  /branches/12/apps/app_voicemail.c 404134 
  /branches/12/apps/app_meetme.c 404134 
  /branches/12/addons/chan_ooh323.c 404134 
  /branches/12/addons/chan_mobile.c 404134 

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


Testing
---

Placed calls, ran tests, all work as expected.


Thanks,

Joshua Colp

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

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

Re: [asterisk-dev] [Code Review] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[rmudgett]

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

(Updated Dec. 18, 2013, 6:51 p.m.)


Review request for Asterisk Developers.


Changes
---

Address review feedback.


Repository: Asterisk


Description
---

Removed code from the system for normal mailbox handling that appends @default 
to the mailbox identifier if it does not have a context.  The only exception is 
the legacy hasvoicemail users.conf option.  The legacy option will only work 
for app_voicemail mailboxes.  (I'd like to just remove the hasvoicemail option 
since it is usually superceeded by an explicit mailbox option. :) )  The system 
cannot make any assumptions about the format of the mailbox identifer used by 
app_voicemail.

chan_sip and chan_dahdi/sig_pri had the most changes because they both tried to 
interpret the mailbox identifier.  chan_sip just stored and compared the two 
components.  chan_dahdi actually used the box information.

The ISDN MWI support configuration options had to be reworked because 
chan_dahdi was parsing the box@context format to get the box number.  As a 
result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
the chan_dahdi.conf.sample file.


Diffs (updated)
-

  /branches/12/res/res_xmpp.c 404197 
  /branches/12/res/res_jabber.c 404197 
  /branches/12/main/app.c 404197 
  /branches/12/include/asterisk/app.h 404197 
  /branches/12/funcs/func_vmcount.c 404197 
  /branches/12/configs/voicemail.conf.sample 404197 
  /branches/12/configs/skinny.conf.sample 404197 
  /branches/12/configs/sip.conf.sample 404197 
  /branches/12/configs/iax.conf.sample 404197 
  /branches/12/configs/chan_dahdi.conf.sample 404197 
  /branches/12/channels/sip/include/sip.h 404197 
  /branches/12/channels/sig_pri.c 404197 
  /branches/12/channels/sig_pri.h 404197 
  /branches/12/channels/h323/chan_h323.h 404197 
  /branches/12/channels/chan_unistim.c 404197 
  /branches/12/channels/chan_skinny.c 404197 
  /branches/12/channels/chan_sip.c 404197 
  /branches/12/channels/chan_mgcp.c 404197 
  /branches/12/channels/chan_iax2.c 404197 
  /branches/12/channels/chan_h323.c 404197 
  /branches/12/channels/chan_dahdi.c 404197 
  /branches/12/channels/chan_dahdi.h 404197 
  /branches/12/apps/app_voicemail.c 404197 
  /branches/12/UPGRADE.txt 404197 
  /branches/12/CHANGES 404197 

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


Testing
---

Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses correctly 
and sets up mailboxes.
Tested chan_sip to be sure that the mailbox option works.


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] 3072: Voicemail: Remove mailbox identifier format (box@context) assumptions in the system.

[rmudgett]

> On Dec. 18, 2013, 2:28 a.m., Matt Jordan wrote:
> > /branches/12/channels/chan_sip.c, line 30853
> > 
> >
> > The length of this should be AST_MAX_EXTENSION + the length of the 
> > context + 2. (Alternatively, use AST_MAX_MAILBOX_UNIQUEID)
> > 
> > app_voicemail uses AST_MAX_EXTENSION as the maximum mailbox name 
> > length, which is 80 characters. Theoretically this could chop the name of 
> > the mailbox down from what is supported in app_voicemail.
> > 
> > Since we haven't defined the length supported by ARI, we might as well 
> > go with what we know is our current max length.

Used AST_MAX_MAILBOX_UNIQUEID for this channel driver and the other channel 
drivers that have a fixed buffer size.


> On Dec. 18, 2013, 2:28 a.m., Matt Jordan wrote:
> > /branches/12/funcs/func_vmcount.c, lines 51-68
> > 
> >
> > This change should be documented in CHANGES/UPGRADE as well

Done.  Though it doesn't really matter.  The only difference is where the 
@default for an app_voicemail mailbox is added.  If a user doesn't add the 
@default for an app_voicemail mailbox it will still be added by app_voicemail.


- rmudgett


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


On Dec. 13, 2013, 11:45 p.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3072/
> ---
> 
> (Updated Dec. 13, 2013, 11:45 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Removed code from the system for normal mailbox handling that appends 
> @default to the mailbox identifier if it does not have a context.  The only 
> exception is the legacy hasvoicemail users.conf option.  The legacy option 
> will only work for app_voicemail mailboxes.  (I'd like to just remove the 
> hasvoicemail option since it is usually superceeded by an explicit mailbox 
> option. :) )  The system cannot make any assumptions about the format of the 
> mailbox identifer used by app_voicemail.
> 
> chan_sip and chan_dahdi/sig_pri had the most changes because they both tried 
> to interpret the mailbox identifier.  chan_sip just stored and compared the 
> two components.  chan_dahdi actually used the box information.
> 
> The ISDN MWI support configuration options had to be reworked because 
> chan_dahdi was parsing the box@context format to get the box number.  As a 
> result the mwi_vm_boxes chan_dahdi.conf option was added and is documented in 
> the chan_dahdi.conf.sample file.
> 
> 
> Diffs
> -
> 
>   /branches/12/res/res_xmpp.c 403807 
>   /branches/12/res/res_jabber.c 403807 
>   /branches/12/main/app.c 403807 
>   /branches/12/include/asterisk/app.h 403807 
>   /branches/12/funcs/func_vmcount.c 403807 
>   /branches/12/configs/voicemail.conf.sample 403807 
>   /branches/12/configs/skinny.conf.sample 403807 
>   /branches/12/configs/sip.conf.sample 403807 
>   /branches/12/configs/iax.conf.sample 403807 
>   /branches/12/configs/chan_dahdi.conf.sample 403807 
>   /branches/12/channels/sip/include/sip.h 403807 
>   /branches/12/channels/sig_pri.c 403807 
>   /branches/12/channels/sig_pri.h 403807 
>   /branches/12/channels/chan_unistim.c 403807 
>   /branches/12/channels/chan_skinny.c 403807 
>   /branches/12/channels/chan_sip.c 403807 
>   /branches/12/channels/chan_mgcp.c 403807 
>   /branches/12/channels/chan_iax2.c 403807 
>   /branches/12/channels/chan_h323.c 403807 
>   /branches/12/channels/chan_dahdi.c 403807 
>   /branches/12/apps/app_voicemail.c 403807 
>   /branches/12/UPGRADE.txt 403807 
>   /branches/12/CHANGES 403807 
> 
> Diff: https://reviewboard.asterisk.org/r/3072/diff/
> 
> 
> Testing
> ---
> 
> Tested chan_dahdi to be sure that the new mwi_vm_boxes option parses 
> correctly and sets up mailboxes.
> Tested chan_sip to be sure that the mailbox option works.
> 
> 
> 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] 3067: channels: Return channel locked when allocating.

[rmudgett]

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

Ship it!


Ship It!

- rmudgett


On Dec. 18, 2013, 5:31 p.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3067/
> ---
> 
> (Updated Dec. 18, 2013, 5:31 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Currently when allocating a channel it is possible for external entities to 
> become aware of the channel and try to interact with it before it is 
> completely set up. This occurs because the channel is returned unlocked, and 
> the caller allocating it is then expected to lock, populate it, and unlock. 
> This leaves a small window where something else can get the channel.
> 
> The attached change makes it so ast_channel_alloc returns the allocated 
> channel locked. It is expected that the caller then populates and unlocks. 
> This leaves no window for something external to get and use it.
> 
> 
> Diffs
> -
> 
>   /branches/12/tests/test_voicemail_api.c 404134 
>   /branches/12/tests/test_substitution.c 404134 
>   /branches/12/tests/test_stasis_channels.c 404134 
>   /branches/12/tests/test_cel.c 404134 
>   /branches/12/tests/test_cdr.c 404134 
>   /branches/12/tests/test_app.c 404134 
>   /branches/12/res/res_stasis_snoop.c 404134 
>   /branches/12/res/res_calendar.c 404137 
>   /branches/12/res/parking/parking_tests.c 404134 
>   /branches/12/main/pbx.c 404134 
>   /branches/12/main/message.c 404134 
>   /branches/12/main/core_unreal.c 404134 
>   /branches/12/main/channel.c 404134 
>   /branches/12/include/asterisk/channel.h 404134 
>   /branches/12/channels/chan_vpb.cc 404134 
>   /branches/12/channels/chan_unistim.c 404134 
>   /branches/12/channels/chan_skinny.c 404134 
>   /branches/12/channels/chan_sip.c 404134 
>   /branches/12/channels/chan_pjsip.c 404134 
>   /branches/12/channels/chan_phone.c 404134 
>   /branches/12/channels/chan_oss.c 404134 
>   /branches/12/channels/chan_nbs.c 404134 
>   /branches/12/channels/chan_multicast_rtp.c 404134 
>   /branches/12/channels/chan_motif.c 404134 
>   /branches/12/channels/chan_misdn.c 404134 
>   /branches/12/channels/chan_mgcp.c 404134 
>   /branches/12/channels/chan_jingle.c 404134 
>   /branches/12/channels/chan_iax2.c 404134 
>   /branches/12/channels/chan_h323.c 404134 
>   /branches/12/channels/chan_gtalk.c 404134 
>   /branches/12/channels/chan_dahdi.c 404134 
>   /branches/12/channels/chan_console.c 404134 
>   /branches/12/channels/chan_alsa.c 404134 
>   /branches/12/apps/confbridge/conf_chan_record.c 404134 
>   /branches/12/apps/app_voicemail.c 404134 
>   /branches/12/apps/app_meetme.c 404134 
>   /branches/12/addons/chan_ooh323.c 404134 
>   /branches/12/addons/chan_mobile.c 404134 
> 
> Diff: https://reviewboard.asterisk.org/r/3067/diff/
> 
> 
> Testing
> ---
> 
> Placed calls, ran tests, all work as expected.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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

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

Re: [asterisk-dev] [Code Review] 3067: channels: Return channel locked when allocating.

[Joshua Colp]

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

(Updated Dec. 18, 2013, 5:31 p.m.)


Review request for Asterisk Developers.


Changes
---

Tweaked core_unreal a bit.


Repository: Asterisk


Description
---

Currently when allocating a channel it is possible for external entities to 
become aware of the channel and try to interact with it before it is completely 
set up. This occurs because the channel is returned unlocked, and the caller 
allocating it is then expected to lock, populate it, and unlock. This leaves a 
small window where something else can get the channel.

The attached change makes it so ast_channel_alloc returns the allocated channel 
locked. It is expected that the caller then populates and unlocks. This leaves 
no window for something external to get and use it.


Diffs (updated)
-

  /branches/12/tests/test_voicemail_api.c 404134 
  /branches/12/tests/test_substitution.c 404134 
  /branches/12/tests/test_stasis_channels.c 404134 
  /branches/12/tests/test_cel.c 404134 
  /branches/12/tests/test_cdr.c 404134 
  /branches/12/tests/test_app.c 404134 
  /branches/12/res/res_stasis_snoop.c 404134 
  /branches/12/res/res_calendar.c 404137 
  /branches/12/res/parking/parking_tests.c 404134 
  /branches/12/main/pbx.c 404134 
  /branches/12/main/message.c 404134 
  /branches/12/main/core_unreal.c 404134 
  /branches/12/main/channel.c 404134 
  /branches/12/include/asterisk/channel.h 404134 
  /branches/12/channels/chan_vpb.cc 404134 
  /branches/12/channels/chan_unistim.c 404134 
  /branches/12/channels/chan_skinny.c 404134 
  /branches/12/channels/chan_sip.c 404134 
  /branches/12/channels/chan_pjsip.c 404134 
  /branches/12/channels/chan_phone.c 404134 
  /branches/12/channels/chan_oss.c 404134 
  /branches/12/channels/chan_nbs.c 404134 
  /branches/12/channels/chan_multicast_rtp.c 404134 
  /branches/12/channels/chan_motif.c 404134 
  /branches/12/channels/chan_misdn.c 404134 
  /branches/12/channels/chan_mgcp.c 404134 
  /branches/12/channels/chan_jingle.c 404134 
  /branches/12/channels/chan_iax2.c 404134 
  /branches/12/channels/chan_h323.c 404134 
  /branches/12/channels/chan_gtalk.c 404134 
  /branches/12/channels/chan_dahdi.c 404134 
  /branches/12/channels/chan_console.c 404134 
  /branches/12/channels/chan_alsa.c 404134 
  /branches/12/apps/confbridge/conf_chan_record.c 404134 
  /branches/12/apps/app_voicemail.c 404134 
  /branches/12/apps/app_meetme.c 404134 
  /branches/12/addons/chan_ooh323.c 404134 
  /branches/12/addons/chan_mobile.c 404134 

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


Testing
---

Placed calls, ran tests, all work as expected.


Thanks,

Joshua Colp

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

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

Re: [asterisk-dev] [Code Review] 3067: channels: Return channel locked when allocating.

[rmudgett]

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



/branches/12/main/core_unreal.c


I think this also needs to be split up and protected by the channel lock to 
protect against very early channel redirects (i.e. masquerade fixups).

Could probably just ignore this as too unlikely to happen.  I'm not sure 
how it could be cleaned up correctly if chan's alocation fails.


- rmudgett


On Dec. 17, 2013, 11:20 p.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3067/
> ---
> 
> (Updated Dec. 17, 2013, 11:20 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Currently when allocating a channel it is possible for external entities to 
> become aware of the channel and try to interact with it before it is 
> completely set up. This occurs because the channel is returned unlocked, and 
> the caller allocating it is then expected to lock, populate it, and unlock. 
> This leaves a small window where something else can get the channel.
> 
> The attached change makes it so ast_channel_alloc returns the allocated 
> channel locked. It is expected that the caller then populates and unlocks. 
> This leaves no window for something external to get and use it.
> 
> 
> Diffs
> -
> 
>   /branches/12/tests/test_voicemail_api.c 404026 
>   /branches/12/tests/test_substitution.c 404026 
>   /branches/12/tests/test_stasis_channels.c 404026 
>   /branches/12/tests/test_cel.c 404026 
>   /branches/12/tests/test_cdr.c 404026 
>   /branches/12/tests/test_app.c 404026 
>   /branches/12/res/res_stasis_snoop.c 404026 
>   /branches/12/res/res_calendar.c 404026 
>   /branches/12/res/parking/parking_tests.c 404026 
>   /branches/12/main/pbx.c 404026 
>   /branches/12/main/message.c 404026 
>   /branches/12/main/core_unreal.c 404026 
>   /branches/12/main/channel.c 404026 
>   /branches/12/include/asterisk/channel.h 404026 
>   /branches/12/channels/chan_vpb.cc 404026 
>   /branches/12/channels/chan_unistim.c 404026 
>   /branches/12/channels/chan_skinny.c 404026 
>   /branches/12/channels/chan_sip.c 404026 
>   /branches/12/channels/chan_pjsip.c 404026 
>   /branches/12/channels/chan_phone.c 404026 
>   /branches/12/channels/chan_oss.c 404026 
>   /branches/12/channels/chan_nbs.c 404026 
>   /branches/12/channels/chan_multicast_rtp.c 404026 
>   /branches/12/channels/chan_motif.c 404026 
>   /branches/12/channels/chan_misdn.c 404026 
>   /branches/12/channels/chan_mgcp.c 404026 
>   /branches/12/channels/chan_jingle.c 404026 
>   /branches/12/channels/chan_iax2.c 404026 
>   /branches/12/channels/chan_h323.c 404026 
>   /branches/12/channels/chan_gtalk.c 404026 
>   /branches/12/channels/chan_dahdi.c 404026 
>   /branches/12/channels/chan_console.c 404026 
>   /branches/12/channels/chan_alsa.c 404026 
>   /branches/12/apps/confbridge/conf_chan_record.c 404026 
>   /branches/12/apps/app_voicemail.c 404026 
>   /branches/12/apps/app_meetme.c 404026 
>   /branches/12/addons/chan_ooh323.c 404026 
>   /branches/12/addons/chan_mobile.c 404026 
> 
> Diff: https://reviewboard.asterisk.org/r/3067/diff/
> 
> 
> Testing
> ---
> 
> Placed calls, ran tests, all work as expected.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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

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

Re: [asterisk-dev] [Code Review] 3057: CDRs: Synchronize dialplan access through Stasis; clean up Answer and DISA

[Joshua Colp]

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

Ship it!


- Joshua Colp


On Dec. 12, 2013, 6:03 p.m., Matt Jordan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3057/
> ---
> 
> (Updated Dec. 12, 2013, 6:03 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22884 and ASTERISK-22886
> https://issues.asterisk.org/jira/browse/ASTERISK-22884
> https://issues.asterisk.org/jira/browse/ASTERISK-22886
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> When doing the rework of the CDR engine that pushed all of the logic into 
> cdr.c and made it respond to changes in channel state over Stasis, I knew 
> that accessing the CDR engine from the dialplan would be "slightly" 
> non-deterministic. Dialplan threads would be accessing CDRs while Stasis 
> threads would be updating the state of said CDRs - whereas in the past, 
> everything happened on the dialplan threads. Tests have shown that "slightly" 
> is in reality "very".
> 
> This patch synchronizes things by making the dialplan applications/functions 
> that manipulate CDRs do so over Stasis. ForkCDR, NoCDR, ResetCDR, CDR, and 
> CDR_PROP now all use Stasis to send their requests over to the CDR engine, 
> and synchronize on the Stasis subscription so that they return their 
> values/control to the dialplan at the appropriate time.
> 
> While going through this, the following changes were also made:
>  * DISA, which can reset the CDR when a user successfully authenticates, now 
> just uses the ResetCDR app to do this. This prevents having to duplicate the 
> same Stasis synchronization logic in that application.
>  * Answer no longer disables CDRs. It actually didn't work anyway - calling 
> DISABLE on the channel's CDR doesn't stop the CDR from getting the Answer 
> time - it just kills all CDRs on that channel, which isn't what the caller 
> would intend.
> 
> 
> Diffs
> -
> 
>   /branches/12/main/pbx.c 403703 
>   /branches/12/main/cdr.c 403703 
>   /branches/12/include/asterisk/cdr.h 403703 
>   /branches/12/funcs/func_cdr.c 403703 
>   /branches/12/apps/app_forkcdr.c 403703 
>   /branches/12/apps/app_disa.c 403703 
>   /branches/12/apps/app_cdr.c 403703 
>   /branches/12/UPGRADE.txt 403703 
>   /branches/12/CHANGES 403703 
> 
> Diff: https://reviewboard.asterisk.org/r/3057/diff/
> 
> 
> Testing
> ---
> 
> * The CDR tests pass (now deterministicly)
> * The hangup handler tests now pass, as they can get reliable values in the h 
> extension from the CDR engine
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

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

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

Re: [asterisk-dev] [Code Review] 3070: bridges: Add two new properties to bridges and bridge snapshots - the name of a creator and the name the creator uses to refer to that bridge

[Jonathan Rose]

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

(Updated Dec. 18, 2013, 12:36 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers, David Lee, kmoore, Mark Michelson, and 
rmudgett.


Repository: Asterisk


Description
---

So there has been at least a little bit of clammoring internally about having 
the need to be able to tell when a bridge belongs to a specific parking lot or 
app_confbridge conference. This patch provides two new properties to the bridge.

BridgeCreator - Provides the name of a system which is responsible for creating 
the bridge.  This includes things such as 'AgentPool', 'Parking', 'ConfBridge', 
'Stasis', and other systems which can create bridges.

BridgeName - Provides the name given to the bridge to refer to it internally by 
the creator.

BridgeCreator may be set or it may be unset (in which case it will appear as a 
zero length string).  BridgeName will only appear when BridgeCreator is also 
set, but it is also optional. So you have the following possibilities:

neither BridgeCreator nor BridgeName
BridgeCreator but not BridgeName
BridgeCreator and BridgeName


Diffs
-

  /branches/12/rest-api/api-docs/bridges.json 403725 
  /branches/12/res/res_stasis.c 403725 
  /branches/12/res/res_ari_bridges.c 403725 
  /branches/12/res/parking/parking_bridge.c 403725 
  /branches/12/res/ari/resource_bridges.c 403725 
  /branches/12/res/ari/resource_bridges.h 403725 
  /branches/12/res/ari/ari_model_validators.c 403725 
  /branches/12/res/ari/ari_model_validators.h 403725 
  /branches/12/main/stasis_bridges.c 403725 
  /branches/12/main/manager_bridges.c 403725 
  /branches/12/main/bridge_basic.c 403725 
  /branches/12/main/bridge.c 403725 
  /branches/12/include/asterisk/stasis_bridges.h 403725 
  /branches/12/include/asterisk/stasis_app.h 403725 
  /branches/12/include/asterisk/bridge_internal.h 403725 
  /branches/12/include/asterisk/bridge.h 403725 
  /branches/12/doc/appdocsxml.xslt 403725 
  /branches/12/apps/app_confbridge.c 403725 
  /branches/12/apps/app_bridgewait.c 403725 
  /branches/12/apps/app_agent_pool.c 403725 

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


Testing
---

Checked output for Manager events displaying Bridge snapshots

Event: BridgeEnter
Privilege: call,all
BridgeUniqueid: a72735f6-04ac-499b-a2da-bb997d6f99ab
BridgeType: parking
BridgeTechnology: holding_bridge
BridgeCreator: Parking
BridgeName: default
BridgeNumChannels: 1
Channel: PJSIP/pjgold-
ChannelState: 6
ChannelStateDesc: Up
CallerIDNum: pjgold
CallerIDName: pjgold
ConnectedLineNum: 
ConnectedLineName: 
AccountCode: 
Context: default
Exten: 700
Priority: 1
Uniqueid: 1386949677.0


Checked output of GET /bridges on ARI in petstore:

[
  {
"id": "a72735f6-04ac-499b-a2da-bb997d6f99ab",
"channels": [],
"technology": "holding_bridge",
"bridge_creator": "Parking",
"bridge_class": "parking",
"bridge_type": "holding",
"bridge_name": "default"
  },
  {
"id": "80e19b8f-5a04-464a-885c-4c119764bf83",
"channels": [
  "1386949828.4"
],
"technology": "holding_bridge",
"bridge_creator": "BridgeWait",
"bridge_class": "base",
"bridge_type": "holding",
"bridge_name": "testname"
  }
]


Thanks,

Jonathan Rose

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

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

Re: [asterisk-dev] [Code Review] 3069: Fix deadlock experienced during multi-party PJSIP transfer through masquerade rework

[Joshua Colp]

> On Dec. 16, 2013, 8:26 p.m., rmudgett wrote:
> > /branches/12/main/channel.c, lines 10399-10400
> > 
> >
> > One thing that ast_channel_masquerade() did that is not being done 
> > anymore is queuing null frames to wake up threads before the masquerade 
> > takes place.
> > 
> > Maybe do_channel_masquerade() needs to poke both channels to wake up 
> > their respective caretaker threads instead.  Those threads could be in the 
> > ast_read/ast_write functions.
> 
> Mark Michelson wrote:
> The old ast_channel_masquerade() would queue null frames, but I 
> interpreted that as being done so that when the thread servicing the channel 
> woke up, the subsequent ast_read() call would end up performing the 
> masquerade. Since we set up and perform the masquerade all at once now, 
> queuing a null frame at the time of marking the channels as being involved in 
> a masquerade seems like the wrong move.
> 
> I *think* that signaling blocking threads is actually taken care of 
> already.
> 
> The clonechan has a null frame queued onto it when it gets marked as a 
> zombie.
> The original channel has a SIGURG sent to its blocking thread shortly 
> after.
> 
> Am I correct that this should result in any blocking threads being awoken 
> correctly?
> 
> rmudgett wrote:
> I was hoping you would know with more certainty than I since the channel 
> fds are swapped around. :)

Yes, it should.


- Joshua


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


On Dec. 12, 2013, 9:59 p.m., Mark Michelson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3069/
> ---
> 
> (Updated Dec. 12, 2013, 9:59 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Jonathan Rose, Matt 
> Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-22936
> https://issues.asterisk.org/jira/browse/ASTERISK-22936
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This is more-or-less a take 2 of https://reviewboard.asterisk.org/r/3042
> 
> Initially, it was discovered that performing an attended transfer of a 
> multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread 
> started a masquerade and reached the point where it was calling the fixup() 
> callback on the "original" channel. For chan_pjsip, this involves pushing a 
> synchronous task to the session's serializer. The problem was that a task 
> ahead of the fixup task was also attempting to perform a channel masquerade. 
> However, since masquerades are designed in a way to only allow for one to 
> occur at a time, the task ahead of the fixup could not continue until the 
> masquerade already in progress had completed. And of course, the masquerade 
> in progress could not complete until the task ahead of the fixup task had 
> completed. Deadlock.
> 
> The initial fix was to change the fixup task to be asynchronous. While this 
> prevented the deadlock from occurring, it had the frightful side effect of 
> potentially allowing for tasks in the session's serializer to operate on a 
> zombie channel.
> 
> Taking a step back from this particular deadlock, it became clear that the 
> problem was not really this one particular issue but that masquerades 
> themselves needed to be addressed. A PJSIP attended transfer operation calls 
> ast_channel_move(), which attempts to both set up and execute a masquerade. 
> The problem was that after it had set up the masquerade, the PBX thread had 
> swooped in and tried to actually perform the masquerade. Looking at changes 
> that had been made to Asterisk 12, it became clear that there never is any 
> time now that anyone ever wants to set up a masquerade and allow for the 
> channel thread to actually perform the masquerade. Everyone always is calling 
> ast_channel_move(), performs the masquerade itself before returning.
> 
> In this patch, I have removed all blocks of code from channel.c that will 
> attempt to perform a masquerade if ast_channel_masq() returns true. Now, 
> there is no distinction between setting up a masquerade and performing the 
> masquerade. It is one operation. The only remaining checks for 
> ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do 
> not want to interrupt a masquerade by hanging up the channel. Instead, now 
> ast_hangup() will wait for a masquerade to complete before moving forward 
> with its operation.
> 
> The ast_channel_move() function has been modified to basically in-line the 
> logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() 
> has been kill

Re: [asterisk-dev] [Code Review] 3067: channels: Return channel locked when allocating.

[Joshua Colp]

> On Dec. 17, 2013, 11:44 p.m., rmudgett wrote:
> > /branches/12/res/res_calendar.c, line 780
> > 
> >
> > Existing bug: chan needs to be locked when adding a datastore.

I've fixed this in 1.8 and up since it was applicable there.


- Joshua


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


On Dec. 17, 2013, 11:20 p.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3067/
> ---
> 
> (Updated Dec. 17, 2013, 11:20 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Currently when allocating a channel it is possible for external entities to 
> become aware of the channel and try to interact with it before it is 
> completely set up. This occurs because the channel is returned unlocked, and 
> the caller allocating it is then expected to lock, populate it, and unlock. 
> This leaves a small window where something else can get the channel.
> 
> The attached change makes it so ast_channel_alloc returns the allocated 
> channel locked. It is expected that the caller then populates and unlocks. 
> This leaves no window for something external to get and use it.
> 
> 
> Diffs
> -
> 
>   /branches/12/tests/test_voicemail_api.c 404026 
>   /branches/12/tests/test_substitution.c 404026 
>   /branches/12/tests/test_stasis_channels.c 404026 
>   /branches/12/tests/test_cel.c 404026 
>   /branches/12/tests/test_cdr.c 404026 
>   /branches/12/tests/test_app.c 404026 
>   /branches/12/res/res_stasis_snoop.c 404026 
>   /branches/12/res/res_calendar.c 404026 
>   /branches/12/res/parking/parking_tests.c 404026 
>   /branches/12/main/pbx.c 404026 
>   /branches/12/main/message.c 404026 
>   /branches/12/main/core_unreal.c 404026 
>   /branches/12/main/channel.c 404026 
>   /branches/12/include/asterisk/channel.h 404026 
>   /branches/12/channels/chan_vpb.cc 404026 
>   /branches/12/channels/chan_unistim.c 404026 
>   /branches/12/channels/chan_skinny.c 404026 
>   /branches/12/channels/chan_sip.c 404026 
>   /branches/12/channels/chan_pjsip.c 404026 
>   /branches/12/channels/chan_phone.c 404026 
>   /branches/12/channels/chan_oss.c 404026 
>   /branches/12/channels/chan_nbs.c 404026 
>   /branches/12/channels/chan_multicast_rtp.c 404026 
>   /branches/12/channels/chan_motif.c 404026 
>   /branches/12/channels/chan_misdn.c 404026 
>   /branches/12/channels/chan_mgcp.c 404026 
>   /branches/12/channels/chan_jingle.c 404026 
>   /branches/12/channels/chan_iax2.c 404026 
>   /branches/12/channels/chan_h323.c 404026 
>   /branches/12/channels/chan_gtalk.c 404026 
>   /branches/12/channels/chan_dahdi.c 404026 
>   /branches/12/channels/chan_console.c 404026 
>   /branches/12/channels/chan_alsa.c 404026 
>   /branches/12/apps/confbridge/conf_chan_record.c 404026 
>   /branches/12/apps/app_voicemail.c 404026 
>   /branches/12/apps/app_meetme.c 404026 
>   /branches/12/addons/chan_ooh323.c 404026 
>   /branches/12/addons/chan_mobile.c 404026 
> 
> Diff: https://reviewboard.asterisk.org/r/3067/diff/
> 
> 
> Testing
> ---
> 
> Placed calls, ran tests, all work as expected.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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

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