From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755454AbcEaQwa (ORCPT ); Tue, 31 May 2016 12:52:30 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:33154 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751616AbcEaQw2 (ORCPT ); Tue, 31 May 2016 12:52:28 -0400 Subject: Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set To: Arnaldo Carvalho de Melo References: <1464657228-24170-1-git-send-email-treeze.taeung@gmail.com> <1464657228-24170-2-git-send-email-treeze.taeung@gmail.com> <20160531134324.GJ2563@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Jiri Olsa From: Taeung Song Message-ID: <574DC147.3040408@gmail.com> Date: Wed, 1 Jun 2016 01:52:23 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160531134324.GJ2563@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 On 05/31/2016 10:43 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu: >> Instead of perf_config(), This function initialize config set >> collecting all configs from config files (i.e. user config >> ~/.perfconfig and system config $(sysconfdir)/perfconfig). >> >> If there are the same config variable both user and system >> config file, user config has higher priority than system config. >> >> Cc: Namhyung Kim >> Cc: Jiri Olsa >> Cc: Masami Hiramatsu >> Cc: Alexander Shishkin >> Signed-off-by: Taeung Song >> --- >> tools/perf/util/config.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 49 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >> index dad7d82..5d01899 100644 >> --- a/tools/perf/util/config.c >> +++ b/tools/perf/util/config.c >> @@ -645,13 +645,61 @@ out_free: >> return -1; >> } >> >> +static int perf_config_set__init(struct perf_config_set *set) >> +{ >> + int ret = 0, found = 0; >> + const char *home = NULL; >> + >> + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ >> + if (config_exclusive_filename) >> + return perf_config_from_file(collect_config, config_exclusive_filename, set); >> + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { >> + ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); >> + found += 1; >> + } >> + >> + home = getenv("HOME"); >> + if (perf_config_global() && home) { >> + char *user_config = strdup(mkpath("%s/.perfconfig", home)); >> + struct stat st; >> + >> + if (user_config == NULL) { >> + warning("Not enough memory to process %s/.perfconfig, " >> + "ignoring it.", home); >> + goto out; >> + } >> + >> + if (stat(user_config, &st) < 0) >> + goto out_free; >> + >> + if (st.st_uid && (st.st_uid != geteuid())) { >> + warning("File %s not owned by current user or root, " >> + "ignoring it.", user_config); >> + goto out_free; >> + } >> + >> + if (!st.st_size) >> + goto out_free; >> + >> + ret += perf_config_from_file(collect_config, user_config, set); >> + found += 1; >> +out_free: >> + free(user_config); >> + } >> +out: >> + if (found == 0) >> + return -1; >> + return ret; >> +} >> + >> struct perf_config_set *perf_config_set__new(void) >> { >> struct perf_config_set *set = zalloc(sizeof(*set)); >> >> if (set) { >> INIT_LIST_HEAD(&set->sections); >> - perf_config(collect_config, set); >> + if (perf_config_set__init(set) < 0) >> + return NULL; > > So, the usual pattern is: alloc, init, fail? free, return NULL. > > I thought you could've been deviating from that pattern and went to look > at perf_config_set__init() to see if that was doing the freeing in case > of failure, which it shouldn't, it isn't, so I guess this is a leak on > failure, no? > You are right. And I found additional problems. First of all, as you said, if it is failed in perf_config_set__init(), the config set wouldn't be freed so this is a leak on failure. Secondly, if it is failed in perf_parse_file(), perf_parse_file() cannot return because of die() so perf_config_from_file() and perf_config() don't also return. I guess this is abnormal termination without the freeing. (The important point of this problem is die() at perf_parse_file()) Thirdly, there are problems that are related to collect_config(). If perf_config_from_file(collect_config,..) is failed the config set will be freed at collect_config() like below. static int collect_config(const char *var, const char *value, void *perf_config_set) { ... out_free: free(key); perf_config_set__delete(set); return -1; } And then if calling perf_config_from_file(collect_config,..) at perf_config_set__init() again, an error will happen because the config set is NULL at collect_config(). (the error mean NULL pointer exception.) To conclude, First of all, I'll send preparatory PATCH set for this patch to solve the problems i.e. 1) A problem that perf_config() can't return becuase of die() at perf_parse_file() 2) A problem about the freeing config set at collect_config() 3) NULL pointer exception at collect_config() And then I will send changed this patch following above patchset. (to solve a leak when perf_config_set__init() failed) Thanks, Taeung