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

Matt Jordan reviewboard at asterisk.org
Tue Mar 4 16:38:08 CST 2014 

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



/branches/12/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/3191/#comment20684>

    Blob
    
    It looks like there are some other space/tab issues here as well.



/branches/12/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/3191/#comment20685>

    Blob



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20686>

    This should probably just say 'Unique id', since assignedid is the variable name



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20687>

    Same finding here



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20688>

    Unique id



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20689>

    Unique id



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20692>

    Since a swap would only ever occur during a channel masquerade - these operations could probably be combined into one. You would never swap the uniqueid and not swap the linkedid as well (in fact, having them separate led to the bug in the first place)



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20690>

    Blob



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20691>

    Blob



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20695>

    I know why we have to have this function; it's just a bit annoying.
    
    Again, you may want to consider combining these, since they're always called together.
    
    ast_channel_internal_set_fake_ids(struct ast_channel *chan, const char *uniqueid, const char *linkedid)



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20693>

    Blobbity



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3191/#comment20694>

    Blob



/branches/12/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/3191/#comment20696>

    Blob. I suspect this is a tab/spacing problem



/branches/12/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/3191/#comment20697>

    If you wanted to, you could use ast_strlen_zero() here.
    
    Otherwise, I'd go ahead and make sure you have spaces between the operator and the operands.



/branches/12/main/manager.c
<https://reviewboard.asterisk.org/r/3191/#comment20698>

    The fast originate handler is used when Originate is performed asynchronously (Async: True)
    
    As it is here, the user's ChannelId/OtherChannelId headers will be ignored if they pass in Async: True.
    
    The fast originate help struct should be modified to copy over the values, such that the fast_originate function passes the assigned IDs to the pbx routines.



/branches/12/main/manager.c
<https://reviewboard.asterisk.org/r/3191/#comment20699>

    I think it'd be worthwhile to throw a WARNING up if the user provided value(s) exceed the maximum unique ID length.



/branches/12/res/ari/resource_channels.c
<https://reviewboard.asterisk.org/r/3191/#comment20700>

    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.



/branches/12/res/ari/resource_channels.c
<https://reviewboard.asterisk.org/r/3191/#comment20701>

    Here as well, I'd validate the the length is less than the maximum allowed, and at least WARN if it is exceeded.



/branches/12/res/ari/resource_channels.c
<https://reviewboard.asterisk.org/r/3191/#comment20702>

    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?


- Matt Jordan


On March 4, 2014, 2:43 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, 2:43 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 409434 
>   /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/5316235a/attachment-0001.html>

More information about the asterisk-dev mailing list