Thanks for reviewing and testing a patch of mine, again, Yann.
"Yann E. MORIN" <yann.morin.1998@xxxxxxx> writes:
> Dirk, All,
>
> On 2013-05-21 10:54 +0200, Dirk Gouders spake thusly:
>> menu_add_prop() applies upper menus' visibilities to actual prompts
>> by AND-ing the prompts visibilities with the upper menus ones.
> [--SNIP--]
>> This patch fixes this problem by creating copies of the menu's
>> visibility expressions before AND-ing them with the prompt's one.
>>
>> Signed-off-by: Dirk Gouders <dirk@xxxxxxxxxxx>
>> ---
>> scripts/kconfig/menu.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
>> index b5c7d90..567939c 100644
>> --- a/scripts/kconfig/menu.c
>> +++ b/scripts/kconfig/menu.c
>> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
>>
>> /* Apply all upper menus' visibilities to actual prompts. */
>> if(type == P_PROMPT) {
>> + struct expr *dup_expr;
>
> I'd rather this variable defined below:
>
>> struct menu *menu = current_entry;
>>
>> while ((menu = menu->parent) != NULL) {
>
> ... here, in the block where it is used, since it is not relevant
> outside this block.
Indeed, I will fix this.
I also noticed that I should fix some spelling (e.g. side effect instead
of side-effect). I hope it is OK, to wait for your tests before sending
a v2.
>> if (!menu->visibility)
>> continue;
>> + /*
>> + * Do not add a reference to the
>> + * menu's visibility expression but
>> + * use a copy of it. Otherwise the
>> + * expression reduction functions
>> + * will modify expressions that have
>> + * multiple references which can
>> + * cause unwanted side-effects.
>> + */
>> + dup_expr = expr_copy(menu->visibility);
>
> I wonder if/where this should be de-allocated.
Actually, I did not find any piece of code that systematically free()s
the allocated data structures and I also think that it would be good to
have such code, because that would have caused double-free()s and
therfore noticed us immediately when we create multiple references to
expressions.
My plan was, to first to fix this single problem and then take care for
a larger review of dynamically allocated memory.
I tested mconf for example with valgrind and the next thing I planned to
suggest is to make the kconfig code mostly "valgrind-clean". But I
expect this to become a rather extensive change and would like to hear
if others also think it should be done.
>
>> +
>> prop->visible.expr
>> - = expr_alloc_and(prop->visible.expr,
>> - menu->visibility);
>> + = expr_alloc_and(prop->visible.expr, dup_expr);
>> }
>> }
>>
>
> I'm testing it right now. Thanks!
Again, thanks for spending the time.
Dirk
> Regards,
> Yann E. MORIN.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
