Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeinglocal variable on error path
From: Adrian Hunter
Date: Tue Jul 02 2013 - 02:20:12 EST
On 01/07/13 21:46, David Ahern wrote:
>
On 7/1/13 2:01 AM, Adrian Hunter wrote:
>
> On 28/06/13 20:19, David Ahern wrote:
>
>> On 6/28/13 2:43 AM, Adrian Hunter wrote:
>
>>> The list_head is on the stack, so just free the rest of the list.
>
>>>
>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>
>>> ---
>
>>> tools/perf/util/parse-events.c | 7 ++++++-
>
>>> tools/perf/util/parse-events.h | 1 +
>
>>> tools/perf/util/pmu.c | 2 +-
>
>>> 3 files changed, 8 insertions(+), 2 deletions(-)
>
>>>
>
>>> diff --git a/tools/perf/util/parse-events.c
>
>>> b/tools/perf/util/parse-events.c
>
>>> index 995fc25..d9cb055 100644
>
>>> --- a/tools/perf/util/parse-events.c
>
>>> +++ b/tools/perf/util/parse-events.c
>
>>> @@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct
>
>>> parse_events_term **new,
>
>>> term->val.str, term->val.num);
>
>>> }
>
>>>
>
>>> -void parse_events__free_terms(struct list_head *terms)
>
>>> +void parse_events__free_terms_only(struct list_head *terms)
>
>>> {
>
>>> struct parse_events_term *term, *h;
>
>>>
>
>>> list_for_each_entry_safe(term, h, terms, list)
>
>>> free(term);
>
>>> +}
>
>>> +
>
>>> +void parse_events__free_terms(struct list_head *terms)
>
>>> +{
>
>>> + parse_events__free_terms_only(terms);
>
>>>
>
>>> free(terms);
>
>>> }
>
>>
>
>> I still don't understand the reasoning for an _only function. There is only
>
>> 1 place that mallocs the list_head and that 1 user should free its own
>
>> memory. All of the other users pass a stack variable.
>
>
>
> No. See parse-events.y
>
>
Fine. Fix both then. My point is that parse-events.c code should not be
>
freeing memory it does not allocate.
No. Read the code. The 'head' member is shared with other lists. It does
not make sense to turn a tiny bug-fix into such a lot of re-work.
>
>
>
> The list head is defined as a pointer in the YYTYPE stack element:
>
>
>
> %union
>
> {
>
> char *str;
>
> u64 num;
>
> struct list_head *head;
>
> struct parse_events_term *term;
>
> }
>
>
>
> It is malloc'ed when terms are created:
>
>
>
> event_term
>
> {
>
> struct list_head *head = malloc(sizeof(*head));
>
> struct parse_events_term *term = 1ドル;
>
>
>
> ABORT_ON(!head);
>
> INIT_LIST_HEAD(head);
>
> list_add_tail(&term->list, head);
>
> $$ = head;
>
> }
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/