[asterisk-dev] [Code Review] 3191: object uniqueid phases 1-3: ami/ari origination & ami bridge, playback, and snoop

Scott Griepentrog reviewboard at asterisk.org
Tue Mar 4 17:54:52 CST 2014 


> On March 4, 2014, 4:38 p.m., Matt Jordan wrote:
> > /branches/12/res/ari/resource_channels.c, lines 1099-1113
> > <https://reviewboard.asterisk.org/r/3191/diff/10/?file=55326#file55326line1099>
> >
> >     I'm not sure I understand the need for the two struct types.
> >     
> >     Why wouldn't just one suffice, and why wouldn't a routine just check to see if the value was set in the struct?

The args list structs are built by swagger from the resource.json description of the rest api - and each has to be a different name in order to prevent collisions.  Also, the contents of the args lists *can* be different, although in this case they differ only in order.

Although this seems a bit convoluted, it's the best approach to map potentially different argument list items and order into the fixed handler, and will fail gracefully with compiler warnings when someone changes the json definition rather than buggy behavior from trying to map the structs to each other.


> On March 4, 2014, 4:38 p.m., Matt Jordan wrote:
> > /branches/12/res/ari/resource_channels.c, lines 753-765
> > <https://reviewboard.asterisk.org/r/3191/diff/10/?file=55326#file55326line753>
> >
> >     What was the reason for breaking apart ast_ari_channels_originate_args?
> >     
> >     It's probably easier to pass that around then all of these separate values.

Necessary based on having two different structs (defined automatically by resource.json file via swagger) that have to feed into a single implementation (handler). 


- Scott


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


On March 4, 2014, 5:54 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3191/
> -----------------------------------------------------------
> 
> (Updated March 4, 2014, 5:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23120
>     https://issues.asterisk.org/jira/browse/ASTERISK-23120
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is the first and second phase of channel uniqueid changes for ASTERISK-23120, including AMI origination UniqueId parameter to assign id on creation.
> 
> * ast_channel_uniqueid structure replaces ast_string values uniqueid and linkedid in channel structure
> 
>     struct ast_channel_id {
>       char unique_id[AST_MAX_UNIQUEID];   /*!< Unique Identifier - can be set on originate */
>       time_t creation_time;               /*!< Creation time */
>       int creation_unique;                /*!< sub-second unique value */
>     }
> 
> * ast_channel_linkedid() and ast_channel_uniqueid() return char* as before, referencing only unique_id element
> 
> * ast_channel_xxxxxxid_set() are removed, new ast_channel_internal_xxx() functions added to provided needed functions regarding entire ast_channel_id structure
> 
> * an issue with argument order to ast_channel_alloc() in chan_mgcp.c was corrected [BUGFIX].
> 
> * an issue with argument order to ast_channel_alloc() in chan_gtalk.c was corrected [BUGFIX].
> 
> * new parameter const char *assignedid added to chain of functions used to create channels (including all channel drivers xxx_request())
> 
> * where channel drivers took struct ast_channel *reqeustor and converted it to linkedid string, these now pass channel as requestor all the way through to ast_channel_alloc so that channel_internal can copy full ast_channel_id linkedid structure with zero penalty.
> 
> * on channel masquerade, when uniqueid is swapped, linkedid is now also swapped [BUGFIX].
> 
> * new struct ast_assigned_ids to pass two sets of uniqueid strings down to core_unreal (Local channels) which replaces char *assignedid in all places.
> 
> * second id defaults to first;2 if not provided
> 
> * now includes phase 3 ari changes (including bridge previously at https://reviewboard.asterisk.org/r/3278/)
> 
> * recording already uses file name as unique id, thus no changes required
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_voicemail_api.c 409434 
>   /branches/12/tests/test_substitution.c 409434 
>   /branches/12/tests/test_stasis_endpoints.c 409434 
>   /branches/12/tests/test_stasis_channels.c 409434 
>   /branches/12/tests/test_cel.c 409434 
>   /branches/12/tests/test_cdr.c 409434 
>   /branches/12/tests/test_app.c 409434 
>   /branches/12/rest-api/api-docs/channels.json 409434 
>   /branches/12/rest-api/api-docs/bridges.json 409434 
>   /branches/12/res/stasis/control.c 409434 
>   /branches/12/res/res_stasis_snoop.c 409434 
>   /branches/12/res/res_stasis_playback.c 409434 
>   /branches/12/res/res_stasis.c 409434 
>   /branches/12/res/res_clioriginate.c 409434 
>   /branches/12/res/res_calendar_icalendar.c 409434 
>   /branches/12/res/res_calendar_exchange.c 409434 
>   /branches/12/res/res_calendar_ews.c 409434 
>   /branches/12/res/res_calendar_caldav.c 409434 
>   /branches/12/res/res_calendar.c 409434 
>   /branches/12/res/res_ari_channels.c 409434 
>   /branches/12/res/res_ari_bridges.c 409434 
>   /branches/12/res/parking/parking_tests.c 409434 
>   /branches/12/res/parking/parking_bridge_features.c 409434 
>   /branches/12/res/parking/parking_bridge.c 409434 
>   /branches/12/res/parking/parking_applications.c 409434 
>   /branches/12/res/ari/resource_channels.c 409434 
>   /branches/12/res/ari/resource_channels.h 409434 
>   /branches/12/res/ari/resource_bridges.c 409434 
>   /branches/12/res/ari/resource_bridges.h 409434 
>   /branches/12/pbx/pbx_spool.c 409434 
>   /branches/12/main/pbx.c 409434 
>   /branches/12/main/message.c 409434 
>   /branches/12/main/manager.c 409434 
>   /branches/12/main/dial.c 409434 
>   /branches/12/main/core_unreal.c 409434 
>   /branches/12/main/core_local.c 409434 
>   /branches/12/main/channel_internal_api.c 409434 
>   /branches/12/main/channel.c 409434 
>   /branches/12/main/cel.c 409434 
>   /branches/12/main/ccss.c 409434 
>   /branches/12/main/bridge_channel.c 409434 
>   /branches/12/main/bridge_basic.c 409434 
>   /branches/12/main/bridge.c 409434 
>   /branches/12/include/asterisk/stasis_app_snoop.h 409434 
>   /branches/12/include/asterisk/stasis_app_playback.h 409434 
>   /branches/12/include/asterisk/stasis_app.h 409434 
>   /branches/12/include/asterisk/pbx.h 409434 
>   /branches/12/include/asterisk/dial.h 409434 
>   /branches/12/include/asterisk/core_unreal.h 409434 
>   /branches/12/include/asterisk/channel_internal.h 409434 
>   /branches/12/include/asterisk/channel.h 409434 
>   /branches/12/include/asterisk/bridge_internal.h 409434 
>   /branches/12/include/asterisk/bridge.h 409434 
>   /branches/12/channels/chan_vpb.cc 409434 
>   /branches/12/channels/chan_unistim.c 409434 
>   /branches/12/channels/chan_skinny.c 409434 
>   /branches/12/channels/chan_sip.c 409434 
>   /branches/12/channels/chan_pjsip.c 409434 
>   /branches/12/channels/chan_phone.c 409434 
>   /branches/12/channels/chan_oss.c 409434 
>   /branches/12/channels/chan_nbs.c 409434 
>   /branches/12/channels/chan_multicast_rtp.c 409434 
>   /branches/12/channels/chan_motif.c 409434 
>   /branches/12/channels/chan_misdn.c 409434 
>   /branches/12/channels/chan_mgcp.c 409434 
>   /branches/12/channels/chan_jingle.c 409434 
>   /branches/12/channels/chan_iax2.c 409434 
>   /branches/12/channels/chan_h323.c 409434 
>   /branches/12/channels/chan_gtalk.c 409434 
>   /branches/12/channels/chan_dahdi.c 409434 
>   /branches/12/channels/chan_console.c 409434 
>   /branches/12/channels/chan_bridge_media.c 409434 
>   /branches/12/channels/chan_alsa.c 409434 
>   /branches/12/apps/confbridge/conf_chan_record.c 409434 
>   /branches/12/apps/confbridge/conf_chan_announce.c 409434 
>   /branches/12/apps/app_voicemail.c 409434 
>   /branches/12/apps/app_queue.c 409434 
>   /branches/12/apps/app_page.c 409434 
>   /branches/12/apps/app_originate.c 409434 
>   /branches/12/apps/app_meetme.c 409434 
>   /branches/12/apps/app_followme.c 409434 
>   /branches/12/apps/app_dial.c 409434 
>   /branches/12/apps/app_confbridge.c 409434 
>   /branches/12/apps/app_chanisavail.c 409434 
>   /branches/12/apps/app_bridgewait.c 409434 
>   /branches/12/apps/app_agent_pool.c 409434 
>   /branches/12/addons/chan_ooh323.c 409678 
>   /branches/12/addons/chan_mobile.c 409434 
> 
> Diff: https://reviewboard.asterisk.org/r/3191/diff/
> 
> 
> Testing
> -------
> 
> Ran new linkedid_check test and received same results.  Also ran some bridge tests to check for asserts.
> 
> Also ran new Originate with assignedid test: https://reviewboard.asterisk.org/r/3243/
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140304/16bcc5b6/attachment-0001.html>

More information about the asterisk-dev mailing list