[asterisk-dev] [Code Review] 3975: Menuselect: Fix incorrect enabling on failed deps

[opticron]

> On Sept. 4, 2014, 10:59 a.m., Matt Jordan wrote:
> > trunk/menuselect.c, lines 632-642
> > <https://reviewboard.asterisk.org/r/3975/diff/1/?file=67200#file67200line632>
> >
> >     I'm not sure I understand why this was broken.
> >     
> >     was_defaulted simply tracks whether or not there is a defaultenabled field. The first condition in this if statement is correct: if was_defaulted is true and there were no dependency failures, then we should set enabled based on the defaultenabled field.
> >     
> >     The problem seems to be in the second clause. If interactive is true, was_enabled should still be False - none of these modules should ever have been enabled. Your change makes us simply ignore was_enabled, and I'm not sure that's correct - or at least I don't understand what all will occur if we do ignore it.
> >     
> >     When I enabled menuselect debugging, I noticed that the code of lines 458 was spitting out "Enabled (module) because the category does not have positive output". That means we are - for some reason - setting was_enabled on disabled modules when they were never enabled at all. I suspect that's more the cause of this issue then the logic here.

The debug line about positive output is valid because it's defined for all channel drivers as part of the MENUSELECT_CHANNELS category. It just defines which subset of items menuselect outputs (selected or not selected) and depends on how those who wrote the makefile chose to handle menuselect output (list of things to process or list of things to exclude from processing). I'll look for a better way to fix the bug.


- opticron


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


On Sept. 4, 2014, 9:55 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3975/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2014, 9:55 a.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Repository: Menuselect
> 
> 
> Description
> -------
> 
> This corrects a situation where menuselect can incorrectly enable a module by default that has defaultenabled set to "no" and has failed/non-selected dependencies.
> 
> 
> Diffs
> -----
> 
>   trunk/menuselect.c 1186 
> 
> Diff: https://reviewboard.asterisk.org/r/3975/diff/
> 
> 
> Testing
> -------
> 
> Verified that the four modules that fit the requirements for this bug were correctly not enabled when the patch was applied. The modules where I observed this behavior are pbx_ael, chan_jingle, chan_gtalk, and chan_mgcp on the 1.8.28 certified branch.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140904/115df6a1/attachment-0001.html>