[asterisk-dev] [Code Review]: Opaquify ast_channel lists and structs

[Terry Wilson]

> On March 1, 2012, 4:45 p.m., Tilghman Lesher wrote:
> > /trunk/main/channel_internal_api.c, lines 751-754
> > <https://reviewboard.asterisk.org/r/1773/diff/2/?file=25075#file25075line751>
> >
> >     Hopefully, this API is going to be revisited at some point before a major release is made.  There's always been some discussion about the inefficiency of a linked list for variables, and this change, although making channel opaque, does not actually allow the backend of channel variable storage to be changed in any real way.
> >     
> >     It needs additional accessor functions, e.g. ast_channel_variable_name(), ast_channel_variable_value(), ast_channel_variable_attribute(), ast_channel_variable_lookup_next(), etc., with an opaque variable container, so that the implementation can change to e.g. a hash table backend.
> >     
> >     My concern is that while we're going through the pain of changing the core channel API, we should be ensuring this API  actually makes the storage of these values really opaque, instead of just hiding a pointer, as this is now.

This is just the first stage. The point being to make as few semantic changes as humanly possible to get to an opaque structure. After things are opaque, it becomes easier to make the semantic changes a little at a time without worrying about introducing bugs in the middle of a 10k line patch. The next stage is to do more than just making a one-to-one mapping to the internal structure, but instead make the API actually make sense.


- Terry


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


On Feb. 27, 2012, 5:47 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1773/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2012, 5:47 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Opaquify lists and remaining structs. I had do a lot of manual fixing in this one. I also added a SWAP macro in chan_local because I was tired of typing and saw some copy/paste code that could use it. I can type things out if people really want me to. :-p Setters for the structs are just wrappers around memcpy. For the most part I could have done things like *ast_channel_caller(chan1) = *ast_channel_caller(chan2), but ick.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 357092 
>   /trunk/apps/app_alarmreceiver.c 357092 
>   /trunk/apps/app_amd.c 357092 
>   /trunk/apps/app_confbridge.c 357092 
>   /trunk/apps/app_dial.c 357092 
>   /trunk/apps/app_disa.c 357092 
>   /trunk/apps/app_dumpchan.c 357092 
>   /trunk/apps/app_fax.c 357092 
>   /trunk/apps/app_followme.c 357092 
>   /trunk/apps/app_macro.c 357092 
>   /trunk/apps/app_meetme.c 357092 
>   /trunk/apps/app_minivm.c 357092 
>   /trunk/apps/app_mixmonitor.c 357092 
>   /trunk/apps/app_osplookup.c 357092 
>   /trunk/apps/app_parkandannounce.c 357092 
>   /trunk/apps/app_privacy.c 357092 
>   /trunk/apps/app_queue.c 357092 
>   /trunk/apps/app_readexten.c 357092 
>   /trunk/apps/app_rpt.c 357092 
>   /trunk/apps/app_setcallerid.c 357092 
>   /trunk/apps/app_sms.c 357092 
>   /trunk/apps/app_stack.c 357092 
>   /trunk/apps/app_talkdetect.c 357092 
>   /trunk/apps/app_voicemail.c 357092 
>   /trunk/apps/app_while.c 357092 
>   /trunk/apps/app_zapateller.c 357092 
>   /trunk/bridges/bridge_builtin_features.c 357092 
>   /trunk/channels/chan_agent.c 357092 
>   /trunk/channels/chan_console.c 357092 
>   /trunk/channels/chan_dahdi.c 357092 
>   /trunk/channels/chan_gtalk.c 357092 
>   /trunk/channels/chan_h323.c 357092 
>   /trunk/channels/chan_iax2.c 357092 
>   /trunk/channels/chan_jingle.c 357092 
>   /trunk/channels/chan_local.c 357092 
>   /trunk/channels/chan_mgcp.c 357092 
>   /trunk/channels/chan_misdn.c 357092 
>   /trunk/channels/chan_oss.c 357092 
>   /trunk/channels/chan_phone.c 357092 
>   /trunk/channels/chan_sip.c 357092 
>   /trunk/channels/chan_skinny.c 357092 
>   /trunk/channels/chan_unistim.c 357092 
>   /trunk/channels/chan_usbradio.c 357092 
>   /trunk/channels/chan_vpb.cc 357092 
>   /trunk/channels/console_video.c 357092 
>   /trunk/channels/sig_analog.c 357092 
>   /trunk/channels/sig_pri.c 357092 
>   /trunk/channels/sig_ss7.c 357092 
>   /trunk/funcs/func_blacklist.c 357092 
>   /trunk/funcs/func_callerid.c 357092 
>   /trunk/funcs/func_dialplan.c 357092 
>   /trunk/funcs/func_strings.c 357092 
>   /trunk/funcs/func_timeout.c 357092 
>   /trunk/include/asterisk/channel.h 357092 
>   /trunk/include/asterisk/utils.h 357092 
>   /trunk/main/abstract_jb.c 357092 
>   /trunk/main/app.c 357092 
>   /trunk/main/autochan.c 357092 
>   /trunk/main/ccss.c 357092 
>   /trunk/main/cdr.c 357092 
>   /trunk/main/cel.c 357092 
>   /trunk/main/channel.c 357092 
>   /trunk/main/channel_internal_api.c 357092 
>   /trunk/main/cli.c 357092 
>   /trunk/main/dial.c 357092 
>   /trunk/main/features.c 357092 
>   /trunk/main/file.c 357092 
>   /trunk/main/manager.c 357092 
>   /trunk/main/message.c 357092 
>   /trunk/main/pbx.c 357092 
>   /trunk/pbx/pbx_lua.c 357092 
>   /trunk/res/res_agi.c 357092 
>   /trunk/res/res_fax.c 357092 
>   /trunk/res/snmp/agent.c 357092 
>   /trunk/tests/test_substitution.c 357092 
> 
> Diff: https://reviewboard.asterisk.org/r/1773/diff
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> 
> Thanks,
> 
> Terry
> 
>

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