[asterisk-dev] [Code Review] 3191: channel uniqueid phase 1: convert string uniqueid values to structure with time

[rmudgett]

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


Is changing AST_MAX_UNIQUEID and MAX_CHANNEL_ID from 150 really a good idea?  The length was potentialy dependent upon the choice of the system name.  With the impending change of the user choosing the channel uniqueid value it could be longer.  Users have been known to pack other information into strings.


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

    ...purposes of call...



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

    Use sizeof(channelid.unique_id) instead of AST_MAX_UNIQUEID.



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

    Use sizeof(channelid.unique_id) instead of AST_MAX_UNIQUEID.



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

    Parentheses are not necessary.
    tmp_id = *ast_channel_uniqueid(clonechan);



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

    Parenthese are not neede here also.



/branches/12/res/res_stasis_snoop.c
<https://reviewboard.asterisk.org/r/3191/#comment20491>

    This really should be using ast_copy_string().
    
    It was most definitely not safe before this patch and this module should not be assuming that uniqueid is going to fit.


- rmudgett


On Feb. 13, 2014, 2:55 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3191/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2014, 2:55 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 phase of channel uniqueid changes for ASTERISK-23120.
> 
> * 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() now return ptr to struct, not char *
> 
> * all references to uniqueid & linkedid updated to either pass entire structure because full uniqueid with time must be propagated, or just the ->unique_id string element.
> 
> * 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].
> 
> * there should be a slight performance improvement by removing the ast_string handling of id's, but at the cost of +~250 bytes to the channel structure.
> 
> * defines for AST_MAX_UNIQUEID (channel.h) and MAX_CHANNEL_ID (rtp_engine.h) have been changed from 150 to 128 to reduce structure alignment issues (and also just  because 150 is ridiculously large)
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_substitution.c 408019 
>   /branches/12/tests/test_cel.c 408019 
>   /branches/12/tests/test_cdr.c 408019 
>   /branches/12/res/stasis/control.c 408019 
>   /branches/12/res/stasis/app.c 408019 
>   /branches/12/res/snmp/agent.c 408019 
>   /branches/12/res/res_stasis_snoop.c 408019 
>   /branches/12/res/res_stasis_recording.c 408019 
>   /branches/12/res/res_stasis_playback.c 408019 
>   /branches/12/res/res_stasis.c 408019 
>   /branches/12/res/res_pjsip_refer.c 408019 
>   /branches/12/res/res_musiconhold.c 408019 
>   /branches/12/res/res_monitor.c 408019 
>   /branches/12/res/res_fax.c 408019 
>   /branches/12/res/res_agi.c 408019 
>   /branches/12/res/parking/parking_bridge_features.c 408019 
>   /branches/12/res/parking/parking_applications.c 408019 
>   /branches/12/res/ari/resource_channels.c 408019 
>   /branches/12/main/stasis_channels.c 408019 
>   /branches/12/main/stasis_bridges.c 408019 
>   /branches/12/main/pbx.c 408019 
>   /branches/12/main/manager.c 408019 
>   /branches/12/main/features.c 408019 
>   /branches/12/main/endpoints.c 408019 
>   /branches/12/main/core_unreal.c 408019 
>   /branches/12/main/channel_internal_api.c 408019 
>   /branches/12/main/channel.c 408019 
>   /branches/12/main/cel.c 408019 
>   /branches/12/main/bridge_channel.c 408019 
>   /branches/12/include/asterisk/rtp_engine.h 408019 
>   /branches/12/include/asterisk/channel_internal.h 408019 
>   /branches/12/include/asterisk/channel.h 408019 
>   /branches/12/funcs/func_channel.c 408019 
>   /branches/12/channels/chan_unistim.c 408019 
>   /branches/12/channels/chan_skinny.c 408019 
>   /branches/12/channels/chan_sip.c 408019 
>   /branches/12/channels/chan_pjsip.c 408019 
>   /branches/12/channels/chan_phone.c 408019 
>   /branches/12/channels/chan_oss.c 408019 
>   /branches/12/channels/chan_multicast_rtp.c 408019 
>   /branches/12/channels/chan_motif.c 408019 
>   /branches/12/channels/chan_mgcp.c 408019 
>   /branches/12/channels/chan_jingle.c 408019 
>   /branches/12/channels/chan_iax2.c 408019 
>   /branches/12/channels/chan_gtalk.c 408019 
>   /branches/12/channels/chan_console.c 408019 
>   /branches/12/channels/chan_alsa.c 408019 
>   /branches/12/apps/app_voicemail.c 408019 
>   /branches/12/apps/app_queue.c 408019 
>   /branches/12/apps/app_minivm.c 408019 
>   /branches/12/apps/app_followme.c 408019 
>   /branches/12/apps/app_dumpchan.c 408019 
>   /branches/12/apps/app_confbridge.c 408019 
>   /branches/12/apps/app_chanspy.c 408019 
>   /branches/12/addons/chan_ooh323.c 408019 
>   /branches/12/addons/chan_mobile.c 408019 
> 
> 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.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140213/462ec69d/attachment.html>