[asterisk-dev] [Code Review] ADSI and Crypto backwards compatibility with 1.6.2

[Tilghman Lesher]

> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/apps/app_adsiprog.c, lines 1597-1599
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12296#file12296line1597>
> >
> >     In all of these cases, there doesn't seem to be any harm in populating the required_resource field in every build, even if that particular system won't use it. I'd honestly prefer a solution that was derived directly from the MODINFO blocks, so that it couldn't get out of sync, but I understand that would be quite complex to build.

I think we could eventually go for such a goal, but my primary aim was to make it work in the short term with 1.8.


> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/apps/app_followme.c, line 54
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12297#file12297line54>
> >
> >     Unrelated change.

I considered it related, insofar as the inclusion was for a module that used optional_api.  I was mainly looking at determining which modules had optional_api dependencies, and thus determined that this inclusion was superfluous.


> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/apps/app_queue.c, line 60
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12299#file12299line60>
> >
> >     Was this missed in the previous conversion? It doesn't look like this new code makes the dependency less strong... the previous changes to optional_api did that.

Yes, it was missed.


> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/channels/chan_mgcp.c, line 33
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12306#file12306line33>
> >
> >     Is this true? Does res_pktccops use optional_api? If not, then chan_mgcp shouldn't be buildable if res_pktccops isn't built as well. Delaying the problem until runtime (as a loading error) doesn't really help the system administrator much.

Yes, res_pktccops uses optional_api.


> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/main/loader.c, line 429
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12310#file12310line429>
> >
> >     What will happen if loading the dependency module fails here?

The dependent module will fail to load, noisily, same as what would occur today.  Given that this function might be called by a CLI command, that this command fails noisily prior to the dependent module failing to load should provide sufficient warning of a compound failure.


> On 2010-08-25 16:42:39, Kevin Fleming wrote:
> > /branches/1.8/include/asterisk/module.h, lines 245-248
> > <https://reviewboard.asterisk.org/r/876/diff/1/?file=12309#file12309line245>
> >
> >     Make this field already exist; changing the composition of an API structure based on platform dependencies seems very unsafe.

Technically, they're only required when the condition applies; otherwise, they need not exist.  I think if we remove the preprocessor definition, the element should be renamed, to avoid confusion... maybe "required_when_gcc_is_broken"?


- Tilghman


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


On 2010-08-24 21:02:08, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/876/
> -----------------------------------------------------------
> 
> (Updated 2010-08-24 21:02:08)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In 1.6.2 and previous, res_adsi and res_crypto used stub files to allow them not to be loaded.  We got rid of the stubs in favor of the optional_api approach.  Now people who are upgrading, who previously noload'ed those modules AND are on a platform which does not support optional_api (i.e. gcc 4.1) (e.g. RHEL, Centos 5) are complaining that the easy upgrade path is broken.
> 
> This patch forces those broken platforms to automatically load the dependencies, when required by a loaded module.
> 
> 
> This addresses bug 17707.
>     https://issues.asterisk.org/view.php?id=17707
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_adsiprog.c 283524 
>   /branches/1.8/apps/app_followme.c 283524 
>   /branches/1.8/apps/app_getcpeid.c 283524 
>   /branches/1.8/apps/app_queue.c 283524 
>   /branches/1.8/apps/app_speech_utils.c 283524 
>   /branches/1.8/apps/app_stack.c 283524 
>   /branches/1.8/apps/app_voicemail.c 283524 
>   /branches/1.8/channels/chan_agent.c 283524 
>   /branches/1.8/channels/chan_dahdi.c 283524 
>   /branches/1.8/channels/chan_iax2.c 283524 
>   /branches/1.8/channels/chan_mgcp.c 283524 
>   /branches/1.8/channels/chan_sip.c 283524 
>   /branches/1.8/funcs/func_aes.c 283524 
>   /branches/1.8/include/asterisk/module.h 283524 
>   /branches/1.8/main/loader.c 283524 
>   /branches/1.8/pbx/pbx_dundi.c 283524 
>   /branches/1.8/pbx/pbx_loopback.c 283524 
>   /branches/1.8/pbx/pbx_realtime.c 283524 
> 
> Diff: https://reviewboard.asterisk.org/r/876/diff
> 
> 
> Testing
> -------
> 
> noloaded modules on a Centos 5 machine, verified they loaded when a dependent module was loaded, either at boot time, or later, at CLI request (i.e. the 'module load' command).
> 
> 
> Thanks,
> 
> Tilghman
> 
>