[asterisk-dev] [Code Review]: Opaquify remaining fields and move struct ast_channel into main/channel_internal_api.c

Terry Wilson reviewboard at asterisk.org
Fri Mar 9 15:52:02 CST 2012 


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/include/asterisk/channel.h, line 716
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25234#file25234line716>
> >
> >     Make the function parameter const void *data to document what the function expects for the parameter.

ok.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel_internal_api.c, lines 49-55
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25240#file25240line49>
> >
> >     How picky do you want to be with the sort.  The fields below are not all sorted this way.
> >     
> >     Another sorting criteria could be access grouping for better cache performance.  Although, this criteria would be harder to identify.

I just moved the comment with the rest of the struct. We can reorder in a another patch since it isn'r related to opaquification.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel_internal_api.c, lines 905-940
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25240#file25240line905>
> >
> >     Using struct copies instead of memcpy is safer.  The compiler can catch type mismatches and won't screw up on the size.  At least it shouldn't screw up on the size. :)

I could have sworn that I changed those last time. Apparently not. I'll get it this time.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel_internal_api.c, line 104
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25240#file25240line104>
> >
> >     blob

Goodbye, blob.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel.c, line 6720
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25239#file25239line6720>
> >
> >     Comment needs updating.

Updated.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/include/asterisk/channel.h, line 3852
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25234#file25234line3852>
> >
> >     Put that space back in there.

How'd that disappear?! Fixed.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel_internal_api.c, lines 416-427
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25240#file25240line416>
> >
> >     Put semicolons on the end of these lines so they don't look funny and give my editor a headache. :)

Your editor, like your old compiler, is broken! :-p Changified.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel_internal_api.c, lines 434-445
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25240#file25240line434>
> >
> >     Put semicolons on the end of these lines so they don't look funny and give my editor a headache. :)

Your editor, like your old compiler, is still broken! :-p Changified.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/include/asterisk/channel.h, lines 3742-3785
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25234#file25234line3742>
> >
> >     It is inconsistent for the getter function to take a const struct ast_channel ptr and return a non-const pointer.

In a perfect world, I would agree. It would be nice for "getters" to take a const struct ast_channel * and return const. But, it is not *yet* a perfect world. Many of the pointers in the ast_channel structure are to variables of types that have their own APIs for manipulation--for example, the ast_format API. To be able to completely opaquify and constify things like internal pointers to ast_format structs, we would need to create ast_channel-specific functions wrapping tons of the other API funcions in Asterisk. Since we are avoiding semantic changes as much as possible at this stage, and instead just making a thin wrapper of the internals, this kind of change doesn't belong in this round of patches.

As to returning non-const pointers out of a const ast_channel being inconsistent, it really (technically) isn't. const struct ast_channel *chan means we aren't changing any of the values in the struct. We aren't changing the pointer that is stored in the structure, we are just returning its value. If that field's contents were going to be treated as const, the internal field and return type of the accessor would be marked as such. It doesn't mean that we can't change the memory pointed to by that pointer. If it was something like:

struct blah {
   int bar;
};
struct ast_channel {
     struct blah foo;
};

then we couldn't change foo.bar if we had const struct ast_channel. But, if we have:

struct ast_channel {
    struct blah *foo;
};

const struct ast_channel * says nothing at all about the contents of foo being const.

The getters as they are now *are* inconsistent, unfortunately. For instance, fields in the internal channel structure that are structs don't have getters taking a const struct ast_channel * because they can't as their addresses still need to be passed to other APIs for modification.

Until we can schedule time for going in and really removing direct access to these kinds of fields and instead create an API that really hides these internals, this is what we are left with. const, in these cases denotes valuable information as to what can/cannot be done at this time.


> On March 8, 2012, 6:41 p.m., rmudgett wrote:
> > /trunk/main/channel.c, line 937
> > <https://reviewboard.asterisk.org/r/1786/diff/1/?file=25239#file25239line937>
> >
> >     the ast_channel_destructor depends upon the stringfields being uninitialized until later so it can correctly destroy a partially initialized channel structure.  When you alloc and initialize the stringfields at the same time you break this!
> >     
> >     It is probably better to break the ast_channel_internal_alloc/ast_channel_internal_cleanup functions up into:
> >     ast_channel_internal_alloc()
> >     ast_channel_internal_stringfields_init()
> >     ast_channel_internal_stringfields_free()
> >     rather than redesign the channel constructor/destructor to handle partially setup channel structs.

Relying on whether or not stringfields are initialized is still a bit hacky. What do you think about adding a function ast_channel_internal_finalize() declared in channel_internal.h like struct ast_channel *ast_channel_internal_finalize(struct ast_channel *chan) which sets a flag and just returning ast_channel_internal_finalize(tmp) at the end of __ast_channel_alloc_ap? I'll go ahead and do this and you can tell me to do something else if you hate it. :-)


- Terry


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


On March 1, 2012, 4:47 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1786/
> -----------------------------------------------------------
> 
> (Updated March 1, 2012, 4:47 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This should finish up the first stage of opaquification.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_channelredirect.c 357721 
>   /trunk/apps/app_chanspy.c 357721 
>   /trunk/apps/app_confbridge.c 357721 
>   /trunk/apps/app_dial.c 357721 
>   /trunk/apps/app_disa.c 357721 
>   /trunk/apps/app_dumpchan.c 357721 
>   /trunk/apps/app_externalivr.c 357721 
>   /trunk/apps/app_followme.c 357721 
>   /trunk/apps/app_macro.c 357721 
>   /trunk/apps/app_mixmonitor.c 357721 
>   /trunk/apps/app_parkandannounce.c 357721 
>   /trunk/apps/app_queue.c 357721 
>   /trunk/apps/app_speech_utils.c 357721 
>   /trunk/apps/app_stack.c 357721 
>   /trunk/apps/app_talkdetect.c 357721 
>   /trunk/bridges/bridge_multiplexed.c 357721 
>   /trunk/channels/chan_agent.c 357721 
>   /trunk/channels/chan_bridge.c 357721 
>   /trunk/channels/chan_dahdi.c 357721 
>   /trunk/channels/chan_local.c 357721 
>   /trunk/channels/chan_phone.c 357721 
>   /trunk/channels/chan_sip.c 357721 
>   /trunk/channels/chan_unistim.c 357721 
>   /trunk/channels/chan_vpb.cc 357721 
>   /trunk/channels/sig_analog.c 357721 
>   /trunk/include/asterisk/channel.h 357721 
>   /trunk/include/asterisk/channel_internal.h PRE-CREATION 
>   /trunk/main/app.c 357721 
>   /trunk/main/autoservice.c 357721 
>   /trunk/main/bridging.c 357721 
>   /trunk/main/channel.c 357721 
>   /trunk/main/channel_internal_api.c 357721 
>   /trunk/main/cli.c 357721 
>   /trunk/main/features.c 357721 
>   /trunk/main/file.c 357721 
>   /trunk/main/indications.c 357721 
>   /trunk/main/manager.c 357721 
>   /trunk/main/pbx.c 357721 
>   /trunk/main/rtp_engine.c 357721 
>   /trunk/res/res_agi.c 357721 
>   /trunk/res/res_musiconhold.c 357721 
>   /trunk/res/snmp/agent.c 357721 
> 
> Diff: https://reviewboard.asterisk.org/r/1786/diff
> 
> 
> Testing
> -------
> 
> unit tests pass. I think I ran the testsuite on it last night and it passed. I'm running it again right now just to make sure.
> 
> 
> Thanks,
> 
> Terry
> 
>

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

More information about the asterisk-dev mailing list