[asterisk-dev] [Code Review] 2532: Migrate a slew of AMI events over to Stasis-Core

Matt Jordan reviewboard at asterisk.org
Fri May 17 14:22:52 CDT 2013 


> On May 16, 2013, 3:54 p.m., opticron wrote:
> > /team/mjordan/ami-refactoring/main/manager_mwi.c, lines 61-66
> > <https://reviewboard.asterisk.org/r/2532/diff/1/?file=37741#file37741line61>
> >
> >     Something *very* similar to this already exists in manager_channels. It looks like you have a cleaner implementation, though. Look for manager_str_from_json_object.

Migrated to manager; updated to use the foreach macro.


> On May 16, 2013, 3:54 p.m., opticron wrote:
> > /trunk/CHANGES, lines 73-74
> > <https://reviewboard.asterisk.org/r/2532/diff/1/?file=37742#file37742line73>
> >
> >     s/Received/Receive/

Fixed


> On May 16, 2013, 3:54 p.m., opticron wrote:
> > /trunk/apps/app_fax.c, lines 259-270
> > <https://reviewboard.asterisk.org/r/2532/diff/1/?file=37744#file37744line259>
> >
> >     Is there a reason not to embed this directly into the pack call?

It has to be a separate pack call, as the Fax message types expect filenames to be an array. IIRC, you can't pack a nested object.

That being said, it's slightly less work if it is a pack call, so changed to do that.


> On May 16, 2013, 3:54 p.m., opticron wrote:
> > /trunk/include/asterisk/stasis_channels.h, line 181
> > <https://reviewboard.asterisk.org/r/2532/diff/1/?file=37758#file37758line181>
> >
> >     I suggest renaming this to something along the lines of "ast_channel_blob_create_from_cached".  When I first saw the function, I thought the blob itself would be cached along with other channel snapshots.
> 
> opticron wrote:
>     This functionality is also duplicated in David's Stasis-HTTP playback branch.

Correct. It was originally a finding on my chanspy work, which was incorporated into this review.


On May 16, 2013, 3:54 p.m., Matt Jordan wrote:
> > The stasis_mwi_* to ast_mwi_* conversion could just be committed to trunk to prune this review down some.
> > 
> > The changes in pbx_dundi and res_config_odbc look like a merge conflict.

The conversion really occurred because of this patch and is tied into the code changes in app.c. Committing it separately feels like a lot of work for very little pay off.

The pbx_dundi/res_config_odbc stuff: whoops. Removed.


- Matt


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


On May 13, 2013, 5:23 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2532/
> -----------------------------------------------------------
> 
> (Updated May 13, 2013, 5:23 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21462
>     https://issues.asterisk.org/jira/browse/ASTERISK-21462
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch refactors a number of AMI events over to Stasis-Core. This includes:
> * ChanSpyStart/Stop
> * MonitorStart/Stop
> * MusicOnHoldStart/Stop
> * FullyBooted/Reload
> * All VoiceMail/MWI related events
> 
> Some infrastructure changes were also made in order to support the new events. This included:
> * A new generic AMI topic and message type. Generic AMI messages have a JSON blob that conforms to the following:
>  { "type": s, "class_type": s, "event": o}
>  where type is the AMI event type; class_type is the AMI class authorization; event is a sequence of key/value pairs that will be directly outputted to AMI.
> * MWI stasis information was renamed to better reflect the naming conventions used
> * MWI objects can now reference a channel snapshot. Often, an MWI chance occurs because a channel modified the state of a mailbox, and having direct access to a snapshot of the channel at that moment in time is important.
> 
> In general, this patch makes a number of trade-offs between what AMI has historically output and what makes the most sense for Stasis-Core to provide. A good example of this are the number of message types defined for the various media/channel related events, as well as what is published to the generic manager topic versus the already defined system topic. The approach taken here was to push information that only AMI consumes to AMI, and if future consumers want that information, to refactor those messages to another topic at that time.
> 
> 
> Diffs
> -----
> 
>   /team/mjordan/ami-refactoring/main/manager_mwi.c 388381 
>   /trunk/CHANGES 388522 
>   /trunk/apps/app_chanspy.c 388522 
>   /trunk/apps/app_fax.c 388522 
>   /trunk/apps/app_minivm.c 388522 
>   /trunk/apps/app_voicemail.c 388522 
>   /trunk/channels/chan_dahdi.c 388522 
>   /trunk/channels/chan_iax2.c 388522 
>   /trunk/channels/chan_mgcp.c 388522 
>   /trunk/channels/chan_sip.c 388522 
>   /trunk/channels/chan_skinny.c 388522 
>   /trunk/channels/chan_unistim.c 388522 
>   /trunk/channels/sig_pri.c 388522 
>   /trunk/include/asterisk/_private.h 388522 
>   /trunk/include/asterisk/app.h 388522 
>   /trunk/include/asterisk/json.h 388522 
>   /trunk/include/asterisk/manager.h 388522 
>   /trunk/include/asterisk/stasis_channels.h 388522 
>   /trunk/main/app.c 388522 
>   /trunk/main/asterisk.c 388522 
>   /trunk/main/cdr.c 388522 
>   /trunk/main/cli.c 388522 
>   /trunk/main/dnsmgr.c 388522 
>   /trunk/main/enum.c 388522 
>   /trunk/main/json.c 388522 
>   /trunk/main/loader.c 388522 
>   /trunk/main/manager.c 388522 
>   /trunk/main/manager_channels.c 388522 
>   /trunk/main/pbx.c 388522 
>   /trunk/main/stasis_channels.c 388522 
>   /trunk/pbx/pbx_dundi.c 388522 
>   /trunk/res/res_config_odbc.c 388522 
>   /trunk/res/res_fax.c 388522 
>   /trunk/res/res_jabber.c 388522 
>   /trunk/res/res_monitor.c 388522 
>   /trunk/res/res_musiconhold.c 388522 
>   /trunk/res/res_sip_mwi.c 388522 
>   /trunk/res/res_xmpp.c 388522 
> 
> Diff: https://reviewboard.asterisk.org/r/2532/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

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

More information about the asterisk-dev mailing list