[asterisk-dev] [Code Review] 3154: CDRs: fix a variety of dial status problems, h/hangup handler creating CDRs, and block shutdown until in flight CDRs are dispatched

[Mark Michelson]

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



/branches/12/apps/app_dial.c
<https://reviewboard.asterisk.org/r/3154/#comment20177>

    I agree with the removal of ast_channel_publish_dial() here. As you pointed out, the status can actually be altered due to events that occur before the parties are bridged.
    
    However, I disagree with the removal of publish_dial_end_event() from here. No matter what ends up happening to the peer, we can accurately publish that the other channels that did not answer have a status of "CANCEL".



/branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/3154/#comment20178>

    Couple of nitpicks:
    
    1) Why do you only examine the first four characters of snapshot->appl? If someone has installed a custom app called "dial_bathsoap" then that app would get locked in. I think you should require a whole string match.
    2) Since the snapshot application comes from a constant from within app_dial, you could drop the case insensitivity and compare case-sensitively to "Dial".


- Mark Michelson


On Jan. 24, 2014, 6:08 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3154/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2014, 6:08 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23164
>     https://issues.asterisk.org/jira/browse/ASTERISK-23164
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes a number of small-ish problems that were noticed when witnessing the records that the FreePBX dialplan produces.
> (1) Mid-call events (as well as privacy options) have the ability to change the overall state of the Dial operation after the called party answers. This means that publishing the DialEnd event when the called party is premature; we have to wait for the execution of these subroutines to complete before we can signal the overall status of the DialEnd. This patch moves that publication and adds handlers for the mid-call events.
> (2) This patch fixes a bug with the F option, where specifying only a priority would be skipped as no '^' character is needed or present. This bug fix will be merged in 1.8+.
> (3) The AST_FLAG_OUTGOING channel flag is cleared if an after bridge goto datastore is detected. This flag was preventing CDRs from being recorded for all outbound channels that had a 'continue' option enabled on them by the Dial application.
> (4) The CDR engine now locks the 'Dial' application as being the CDR application if it detects that the current CDR has entered that app. This is similar to the logic that is done for Parking. In general, if we entered into Dial, then we want that CDR to record the application as such - this prevent pre-dial handlers, mid-call handlers, and other shenaniganry from changing the application value.
> (5) The CDR engine now checks for both the AST_FLAG_DEAD and the AST_SOFTHANGUP_HANGUP_EXEC to determine if the channel is in hangup logic or dead. In either case, we don't want to record changes in the channel.
> (6) Finally, because we now have the ability to synchronize on the messages published to the CDR topic, on shutdown the CDR engine will now synchronize to the messages currently in flight. This helps to ensure that all in-flight CDRs are written before shutting down.
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/pbx.c 406293 
>   /branches/12/main/manager_channels.c 406293 
>   /branches/12/main/cdr.c 406293 
>   /branches/12/main/bridge_after.c 406293 
>   /branches/12/apps/app_dial.c 406293 
> 
> Diff: https://reviewboard.asterisk.org/r/3154/diff/
> 
> 
> Testing
> -------
> 
> See review https://reviewboard.asterisk.org/r/3153/
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140124/38623ccf/attachment.html>