[asterisk-dev] [Code Review]: Start the process of opaque-ifying the ast_channel by replacing direct access to the 'name' field with accessor functions.

Terry Wilson reviewboard at asterisk.org
Mon Jan 9 11:40:40 CST 2012 


> On Jan. 9, 2012, 6:13 a.m., schmidts wrote:
> > /trunk/main/channel.c, line 1674
> > <https://reviewboard.asterisk.org/r/1655/diff/2/?file=22864#file22864line1674>
> >
> >     see comment above, CMP_MATCH & CMP_STOP
> 
> Terry Wilson wrote:
>     I prefer to let the caller choose via flags whether or not to continue searching for other matches. If they don't specifically pass OBJ_MULTIPLE, I think CMP_MATCH implies stopping. Maybe I should specifically pass 0 for flags when ast_channel_by_uniqueid_cb is called from ast_channel_cmp_cb?
> 
> Mark Michelson wrote:
>     There's no reason to set OBJ_MULTIPLE when calling by uniqueid. When OBJ_MULTIPLE is not supplied, then CMP_MATCH implies CMP_STOP, but it doesn't hurt to have CMP_MATCH | CMP_STOP there. I find it's more expressive, plus (imho) the callback shouldn't be aware what flags have been set and should act based on how it knows its relevant data behaves.

To me, it seems limiting. I like trusting the caller to know what they really want. For example, let's say you wanted to write a test that determines whether or not your uniqueid is "sufficiently unique". Normal code would never call with OBJ_MULTIPLE, but the test code could to find out if there really were more than one match. The only thing you end up doing is limiting what the caller can do with the code. The callback, in this way, can be truly agnostic about the flags and just do what the caller wants and not impose its view of the the world on things. Just my opinion, anyway. If other people feel differently, I am amenable to change.


- Terry


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


On Jan. 6, 2012, 3:51 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1655/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2012, 3:51 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There would be many benefits to making the ast_channel an opaque handle, from increasing maintainability to presenting ways to kill masquerades. I propose that we kick things off by taking things a field at a time, renaming the field to '__do_not_use_${fieldname}' and then writing setters/getters and converting the existing code to using them. When all fields are done, we can move ast_channel to a C file from channel.h and lop off the '__do_not_use_'.
> 
> This patch sets up main/channel_interal_api.c to be the only file that actually accesses the ast_channel's fields directly. The intent would be for any API functions in channel.c to use the accessor functions. No more monkeying around with channel internals. We should use our own APIs.
> 
> The interesting changes in this patch are the addition of channel_internal_api.c, the moving of the AST_DATA stuff from channel.c to channel_internal_api.c (note: the AST_DATA stuff will have to be reworked to use accessor functions when ast_channel is really opaque), and some re-working of the way channel iterators/callbacks are handled so as to avoid creating fake ast_channels on the stack to pass in matching data by directly accessing fields (since "name" is a stringfield and the fake channel doesn't init the stringfields, you can't use the ast_channel_name_set() function). I went with ast_channel_name(chan) for a getter, and ast_channel_name_set(chan, name) for a setter.
> 
> Ideas welcome!
> 
> The majority of the grunt-work for this change was done by writing a semantic patch using Coccinelle ( http://coccinelle.lip6.fr/ ).
> 
> note: There are some extra changes to some of the h323 files to comment out variables that were set and not used so that I could get it to compile under dev-mode.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_mobile.c 349871 
>   /trunk/addons/chan_ooh323.c 349871 
>   /trunk/addons/ooh323c/src/memheap.c 349871 
>   /trunk/apps/app_adsiprog.c 349871 
>   /trunk/apps/app_alarmreceiver.c 349871 
>   /trunk/apps/app_amd.c 349871 
>   /trunk/apps/app_chanisavail.c 349871 
>   /trunk/apps/app_chanspy.c 349871 
>   /trunk/apps/app_confbridge.c 349871 
>   /trunk/apps/app_dahdibarge.c 349871 
>   /trunk/apps/app_dahdiras.c 349871 
>   /trunk/apps/app_dial.c 349871 
>   /trunk/apps/app_directed_pickup.c 349871 
>   /trunk/apps/app_disa.c 349871 
>   /trunk/apps/app_dumpchan.c 349871 
>   /trunk/apps/app_externalivr.c 349871 
>   /trunk/apps/app_fax.c 349871 
>   /trunk/apps/app_flash.c 349871 
>   /trunk/apps/app_followme.c 349871 
>   /trunk/apps/app_getcpeid.c 349871 
>   /trunk/apps/app_jack.c 349871 
>   /trunk/apps/app_macro.c 349871 
>   /trunk/apps/app_meetme.c 349871 
>   /trunk/apps/app_milliwatt.c 349871 
>   /trunk/apps/app_minivm.c 349871 
>   /trunk/apps/app_mixmonitor.c 349871 
>   /trunk/apps/app_page.c 349871 
>   /trunk/apps/app_parkandannounce.c 349871 
>   /trunk/apps/app_playback.c 349871 
>   /trunk/apps/app_queue.c 349871 
>   /trunk/apps/app_readexten.c 349871 
>   /trunk/apps/app_record.c 349871 
>   /trunk/apps/app_rpt.c 349871 
>   /trunk/apps/app_sms.c 349871 
>   /trunk/apps/app_softhangup.c 349871 
>   /trunk/apps/app_stack.c 349871 
>   /trunk/apps/app_talkdetect.c 349871 
>   /trunk/apps/app_test.c 349871 
>   /trunk/apps/app_voicemail.c 349871 
>   /trunk/apps/app_waitforsilence.c 349871 
>   /trunk/bridges/bridge_multiplexed.c 349871 
>   /trunk/channels/chan_agent.c 349871 
>   /trunk/channels/chan_alsa.c 349871 
>   /trunk/channels/chan_console.c 349871 
>   /trunk/channels/chan_dahdi.c 349871 
>   /trunk/channels/chan_gtalk.c 349871 
>   /trunk/channels/chan_h323.c 349871 
>   /trunk/channels/chan_iax2.c 349871 
>   /trunk/channels/chan_jingle.c 349871 
>   /trunk/channels/chan_local.c 349871 
>   /trunk/channels/chan_mgcp.c 349871 
>   /trunk/channels/chan_misdn.c 349871 
>   /trunk/channels/chan_nbs.c 349871 
>   /trunk/channels/chan_oss.c 349871 
>   /trunk/channels/chan_phone.c 349871 
>   /trunk/channels/chan_sip.c 349871 
>   /trunk/channels/chan_skinny.c 349871 
>   /trunk/channels/chan_unistim.c 349871 
>   /trunk/channels/chan_usbradio.c 349871 
>   /trunk/channels/chan_vpb.cc 349871 
>   /trunk/channels/console_video.c 349871 
>   /trunk/channels/sig_analog.c 349871 
>   /trunk/channels/sig_pri.c 349871 
>   /trunk/channels/sig_ss7.c 349871 
>   /trunk/funcs/func_audiohookinherit.c 349871 
>   /trunk/funcs/func_channel.c 349871 
>   /trunk/funcs/func_frame_trace.c 349871 
>   /trunk/funcs/func_global.c 349871 
>   /trunk/funcs/func_groupcount.c 349871 
>   /trunk/funcs/func_lock.c 349871 
>   /trunk/include/asterisk/channel.h 349871 
>   /trunk/main/abstract_jb.c 349871 
>   /trunk/main/aoc.c 349871 
>   /trunk/main/app.c 349871 
>   /trunk/main/autochan.c 349871 
>   /trunk/main/bridging.c 349871 
>   /trunk/main/ccss.c 349871 
>   /trunk/main/cdr.c 349871 
>   /trunk/main/cel.c 349871 
>   /trunk/main/channel.c 349871 
>   /trunk/main/channel_internal_api.c PRE-CREATION 
>   /trunk/main/cli.c 349871 
>   /trunk/main/dial.c 349871 
>   /trunk/main/dsp.c 349871 
>   /trunk/main/features.c 349871 
>   /trunk/main/file.c 349871 
>   /trunk/main/indications.c 349871 
>   /trunk/main/manager.c 349871 
>   /trunk/main/pbx.c 349871 
>   /trunk/main/rtp_engine.c 349871 
>   /trunk/main/say.c 349871 
>   /trunk/main/udptl.c 349871 
>   /trunk/pbx/pbx_dundi.c 349871 
>   /trunk/pbx/pbx_lua.c 349871 
>   /trunk/pbx/pbx_realtime.c 349871 
>   /trunk/res/res_adsi.c 349871 
>   /trunk/res/res_agi.c 349871 
>   /trunk/res/res_calendar.c 349871 
>   /trunk/res/res_fax.c 349871 
>   /trunk/res/res_jabber.c 349871 
>   /trunk/res/res_monitor.c 349871 
>   /trunk/res/res_musiconhold.c 349871 
>   /trunk/res/res_mutestream.c 349871 
>   /trunk/res/snmp/agent.c 349871 
> 
> Diff: https://reviewboard.asterisk.org/r/1655/diff
> 
> 
> Testing
> -------
> 
> I ran it through the testsuite and seemed to get the same number of failures that I got without it. I also made some calls and tested out 'core show channel <tab>' and 'core show channel Console/dsp', and core show channels to make sure that the changes to the iterators seemed to work.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120109/372f7bdc/attachment.htm>

More information about the asterisk-dev mailing list