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

Matt Jordan reviewboard at asterisk.org
Thu Feb 20 12:16:01 CST 2014 


> On Feb. 19, 2014, 5:23 p.m., Matt Jordan wrote:
> > /branches/12/main/cel.c, lines 833-842
> > <https://reviewboard.asterisk.org/r/3191/diff/5/?file=54215#file54215line833>
> >
> >     There shouldn't be any reason to memset these, as the structure is calloc'd.
> 
> rmudgett wrote:
>     channelid is a local variable on the stack and not calloced so it needs to be memset.

You're right, my bad. Drop this finding.


> On Feb. 19, 2014, 5:23 p.m., Matt Jordan wrote:
> > /branches/12/include/asterisk/channel.h, lines 178-187
> > <https://reviewboard.asterisk.org/r/3191/diff/5/?file=54211#file54211line178>
> >
> >     This struct should be opaque, in the same way that all other channel internals are also opaque.
> >     
> >     That means directly accessing the unique_id field should not be allowed. Instead, calling ast_channel_uniqueid or ast_channel_linkedid should still return a const char *, that happens to map to the unique_id contained in the structure.
> >     
> >     The fact that we now have a structure should be - almost entirely - an implementation detail of channel.c and internal_channel.c, and not exposed to users of the channel. That's a large part of why we opaquified the channel.
> >     
> >     That will, unfortunately, undo some of the changes you made - but I'd hate to undo the channel opaquification that was done by others in Asterisk 11.
> 
> rmudgett wrote:
>     ast_channel is still opaque.  The ast_channel_id is doing the same thing as the ast_party_caller, ast_party_connected_line, ast_party_dialed, ast_party_redirecting, and some other structs that are in ast_channel.  Though I suppose the ast_channel_id could itself be made opaque with an accessor function for the uniqueid.
>     
>     Another way would be to make ast_channel_uniqueid() and ast_channel_linkedid() return the associated ast_channel_id.unique_id so the callers won't need to be changed.  Then another ast_channel accessor function could be created to return a pointer to the ast_channel_id if it is needed.  Or the ast_channel_id struct may not need to be exposed outside of channel_internal_api.c.
> 
> Scott Griepentrog wrote:
>     Hiding the existence of the time structures to anything outside channel_internal can be done, but would require: a) copy of linkedid from one channel to another to be located in channel_internal as a function, b) the function would have to look up the channel structure by the char*linkedid being passed around to get the entire structure again to copy it, unless the caller has the ptr to the channel structure itself (bridge linkedid propagation does, but creating new channel inheriting another's linkedid doesn't).

A Unique ID for a channel is not the same thing as party information. The unique ID for the channel is logically part of the ast_channel structure - the fact that it involves a timestamp and a string that represents the ID should be an implementation detail. Exposing it allows consumers to abuse that implementation - something that we've fallen into time and again when things on the channel were not opaque.

>From the perspective of users of ast_channel, the unique ID should be *just* the string. The timestamp field only exists so that we can determine the oldest unique ID for linkedid propagation. That's an implementation detail of how channels work, and should be located in either channel.c or channel_internal.c.


- Matt


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


On Feb. 19, 2014, 3:38 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3191/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2014, 3:38 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 408446 
>   /branches/12/tests/test_cel.c 408446 
>   /branches/12/tests/test_cdr.c 408446 
>   /branches/12/res/stasis/control.c 408446 
>   /branches/12/res/stasis/app.c 408446 
>   /branches/12/res/snmp/agent.c 408446 
>   /branches/12/res/res_stasis_snoop.c 408446 
>   /branches/12/res/res_stasis_recording.c 408446 
>   /branches/12/res/res_stasis_playback.c 408446 
>   /branches/12/res/res_stasis.c 408446 
>   /branches/12/res/res_pjsip_refer.c 408446 
>   /branches/12/res/res_musiconhold.c 408446 
>   /branches/12/res/res_monitor.c 408446 
>   /branches/12/res/res_fax.c 408446 
>   /branches/12/res/res_agi.c 408446 
>   /branches/12/res/parking/parking_bridge_features.c 408446 
>   /branches/12/res/parking/parking_applications.c 408446 
>   /branches/12/res/ari/resource_channels.c 408446 
>   /branches/12/main/stasis_channels.c 408446 
>   /branches/12/main/stasis_bridges.c 408446 
>   /branches/12/main/pbx.c 408446 
>   /branches/12/main/manager.c 408446 
>   /branches/12/main/features.c 408446 
>   /branches/12/main/endpoints.c 408446 
>   /branches/12/main/core_unreal.c 408446 
>   /branches/12/main/channel_internal_api.c 408446 
>   /branches/12/main/channel.c 408446 
>   /branches/12/main/cel.c 408446 
>   /branches/12/main/bridge_channel.c 408446 
>   /branches/12/include/asterisk/rtp_engine.h 408446 
>   /branches/12/include/asterisk/channel_internal.h 408446 
>   /branches/12/include/asterisk/channel.h 408446 
>   /branches/12/funcs/func_channel.c 408446 
>   /branches/12/channels/chan_vpb.cc 408446 
>   /branches/12/channels/chan_unistim.c 408446 
>   /branches/12/channels/chan_skinny.c 408446 
>   /branches/12/channels/chan_sip.c 408446 
>   /branches/12/channels/chan_pjsip.c 408446 
>   /branches/12/channels/chan_phone.c 408446 
>   /branches/12/channels/chan_oss.c 408446 
>   /branches/12/channels/chan_nbs.c 408446 
>   /branches/12/channels/chan_multicast_rtp.c 408446 
>   /branches/12/channels/chan_motif.c 408446 
>   /branches/12/channels/chan_misdn.c 408446 
>   /branches/12/channels/chan_mgcp.c 408446 
>   /branches/12/channels/chan_jingle.c 408446 
>   /branches/12/channels/chan_iax2.c 408446 
>   /branches/12/channels/chan_h323.c 408446 
>   /branches/12/channels/chan_gtalk.c 408446 
>   /branches/12/channels/chan_dahdi.c 408446 
>   /branches/12/channels/chan_console.c 408446 
>   /branches/12/channels/chan_alsa.c 408446 
>   /branches/12/apps/app_voicemail.c 408446 
>   /branches/12/apps/app_queue.c 408446 
>   /branches/12/apps/app_minivm.c 408446 
>   /branches/12/apps/app_followme.c 408446 
>   /branches/12/apps/app_dumpchan.c 408446 
>   /branches/12/apps/app_confbridge.c 408446 
>   /branches/12/apps/app_chanspy.c 408446 
>   /branches/12/addons/chan_ooh323.c 408446 
>   /branches/12/addons/chan_mobile.c 408446 
> 
> 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/20140220/cd9a81a3/attachment-0001.html>

More information about the asterisk-dev mailing list