From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751593AbdBAIEQ (ORCPT ); Wed, 1 Feb 2017 03:04:16 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:33022 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751292AbdBAIEP (ORCPT ); Wed, 1 Feb 2017 03:04:15 -0500 Subject: Re: [PATCH v2 3/4] perf tools: Check NULL after zalloc() and Use zfree() instead of free() in parse-events.c To: Arnaldo Carvalho de Melo References: <1485862711-20216-1-git-send-email-treeze.taeung@gmail.com> <1485862711-20216-4-git-send-email-treeze.taeung@gmail.com> <20170131132335.GE4491@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Wang Nan , Jiri Olsa From: Taeung Song Message-ID: <2650b144-8a7a-e902-3012-4d1167e188e4@gmail.com> Date: Wed, 1 Feb 2017 17:04:08 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170131132335.GE4491@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Sorry I'm late.. I got it! I'll send v3 with changed patches !! Thanks, Taeung On 01/31/2017 10:23 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, Jan 31, 2017 at 08:38:30PM +0900, Taeung Song escreveu: >> Currently there are several parts not checking NULL >> after allocating with zalloc() or asigning NULL value >> to a pointer variable after doing free(). >> >> So I fill in code checking NULL and >> use zfree() instead of free(). > > You are doing more than one thing on this patch, don't do that. > > Please split it for the "no functional changes" parts, i.e. things like > > - free(path); > - path = NULL; > + zfree(&path); > > From the ones that _change_ logic, like the first hunk: > > path = zalloc(sizeof(*path)); > + if (!path) > + return NULL; > path->system = malloc(MAX_EVENT_LENGTH); > if (!path->system) { > free(path); > > This one as well changes logic: > > @@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name) > r = bsearch(&p, perf_pmu_events_list, > (size_t) perf_pmu_events_list_num, > sizeof(struct perf_pmu_event_symbol), comp_pmu); > - free(p.symbol); > + zfree(&p.symbol); > return r ? r->type : PMU_EVENT_SYMBOL_ERR; > > >> Cc: Namhyung Kim >> Cc: Jiri Olsa >> Signed-off-by: Taeung Song >> --- >> tools/perf/util/parse-events.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index 3c876b8..87a3e5a 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -211,6 +211,8 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config) >> closedir(evt_dir); >> closedir(sys_dir); >> path = zalloc(sizeof(*path)); >> + if (!path) >> + return NULL; >> path->system = malloc(MAX_EVENT_LENGTH); >> if (!path->system) { >> free(path); >> @@ -252,8 +254,7 @@ struct tracepoint_path *tracepoint_name_to_path(const char *name) >> if (path->system == NULL || path->name == NULL) { >> zfree(&path->system); >> zfree(&path->name); >> - free(path); >> - path = NULL; >> + zfree(&path); >> } >> >> return path; >> @@ -1477,10 +1478,9 @@ static void perf_pmu__parse_cleanup(void) >> >> for (i = 0; i < perf_pmu_events_list_num; i++) { >> p = perf_pmu_events_list + i; >> - free(p->symbol); >> + zfree(&p->symbol); >> } >> - free(perf_pmu_events_list); >> - perf_pmu_events_list = NULL; >> + zfree(&perf_pmu_events_list); >> perf_pmu_events_list_num = 0; >> } >> } >> @@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name) >> r = bsearch(&p, perf_pmu_events_list, >> (size_t) perf_pmu_events_list_num, >> sizeof(struct perf_pmu_event_symbol), comp_pmu); >> - free(p.symbol); >> + zfree(&p.symbol); >> return r ? r->type : PMU_EVENT_SYMBOL_ERR; >> } >> >> @@ -1710,8 +1710,8 @@ static void parse_events_print_error(struct parse_events_error *err, >> fprintf(stderr, "%*s\\___ %s\n", idx + 1, "", err->str); >> if (err->help) >> fprintf(stderr, "\n%s\n", err->help); >> - free(err->str); >> - free(err->help); >> + zfree(&err->str); >> + zfree(&err->help); >> } >> >> fprintf(stderr, "Run 'perf list' for a list of valid events\n"); >> @@ -2406,7 +2406,7 @@ void parse_events_terms__purge(struct list_head *terms) >> >> list_for_each_entry_safe(term, h, terms, list) { >> if (term->array.nr_ranges) >> - free(term->array.ranges); >> + zfree(&term->array.ranges); >> list_del_init(&term->list); >> free(term); >> } >> @@ -2422,7 +2422,7 @@ void parse_events_terms__delete(struct list_head *terms) >> >> void parse_events__clear_array(struct parse_events_array *a) >> { >> - free(a->ranges); >> + zfree(&a->ranges); >> } >> >> void parse_events_evlist_error(struct parse_events_evlist *data, >> -- >> 2.7.4