[asterisk-dev] [Code Review] 2694: Clean up queue operations (get rid of BUGBUGs)

[opticron]

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



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18499>

    The peer and caller snapshots should be handled if NULL in this function as queue_publish_multi_channel_snapshot_blob does not check for NULL snapshots and callers of send_agent_complete could pass NULL snapshots into this function.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18500>

    This should be using a message router.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18503>

    Since this is only used in an embedded fashion (twice) and only has one string field member, you should avoid the overhead of string fields and use an ast_strdup()d pointer.
    
    If it wasn't used in an embedded manner, I'd suggest embedding the string at the end of the struct.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18505>

    Since the channel and bridge unsubscriptions are handled elsewhere, this should assert that both are NULL or handle them properly if they're not.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18504>

    This should be queue_unref()d in a NULL-safe manner instead of ao2_cleanup()d.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18506>

    These should be ast_str_set() instead of ast_str_append().



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18508>

    The inner call to ast_json_object_get needs to be checked for NULL first.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18507>

    This should use a message router.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/2694/#comment18509>

    Convert this to a message router as well.


- opticron


On Aug. 1, 2013, 3:50 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2694/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2013, 3:50 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21517 and ASTERISK-21943
>     https://issues.asterisk.org/jira/browse/ASTERISK-21517
>     https://issues.asterisk.org/jira/browse/ASTERISK-21943
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Large-scale bridge refactoring left app_queue in a state with lots of commented-out code and many BUGBUG comments.
> 
> This changeset aims to improve app_queue to get back to proper operation. The following changes were made:
> 
> * Reporting of transfers and call completion is done by creating stasis subscriptions and listening for specific events in order to determine when the call is finished (either via a transfer or hangup).
> * Dial end messages have been added where they were previously missing.
> * Queue stats are properly being updated again once calls have finished.
> * AgentComplete stasis messages and AMI events are now occurring again.
> * Mixmonitor starting has been factored into its own function and uses the Mixmonitor API now instead of using ast_pbx_run()
> 
> In addition to the changes in app_queue, there are several supplementary changes as well:
> 
> * Queue logging now differentiates between attended and blind transfers. A note about this is in the CHANGES file.
> * Local channel optimization events now report more information. This includes which of the two local channels involved is the destination of the optimization, the channel that is replacing the destination local channel, and an identifier so that begin and end events can be matched to each other. The end events are now sent whether the optimization was successful or not and includes an indicator of whether the optimization was successful.
> * Changes were made to features and bridging_basic so that additional flags may be set on a bridge. This is necessary because the queue requires that its bridge only allows move-swap local channel optimizations into the bridge.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 396027 
>   /trunk/apps/app_queue.c 396027 
>   /trunk/include/asterisk/app.h 396027 
>   /trunk/include/asterisk/bridge_basic.h 396027 
>   /trunk/include/asterisk/core_unreal.h 396027 
>   /trunk/include/asterisk/features.h 396027 
>   /trunk/main/app.c 396027 
>   /trunk/main/bridge.c 396027 
>   /trunk/main/bridge_basic.c 396027 
>   /trunk/main/core_local.c 396027 
>   /trunk/main/features.c 396027 
> 
> Diff: https://reviewboard.asterisk.org/r/2694/diff/
> 
> 
> Testing
> -------
> 
> Loads.
> 
> The biggest thing that needed to be checked was that hangups and transfers were detected properly. I tested various local channel optimization scenarios to ensure that they worked as expected. I also ensured that the mixmonitor changes worked as expected.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130819/e4cbb462/attachment-0001.htm>