From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751753AbcFFLGV (ORCPT ); Mon, 6 Jun 2016 07:06:21 -0400 Received: from mail-pa0-f68.google.com ([209.85.220.68]:35062 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751125AbcFFLGU (ORCPT ); Mon, 6 Jun 2016 07:06:20 -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> <574DC147.3040408@gmail.com> 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: <57555927.8070405@gmail.com> Date: Mon, 6 Jun 2016 20:06:15 +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: <574DC147.3040408@gmail.com> 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 Hi, Arnaldo :) Did you have a nice weekend? I sent this mail for nothing else but to tell the reason of v6 to you. On 06/01/2016 01:52 AM, Taeung Song wrote: > > > 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? I can modify the problem of the leak you said by simple handling a case of failure at perf_config_set__init(). But I found preexisting small problems so I sent v6 with the three [BUGFIX] patches !! If you can't agree this way to solve the leak, I'd find other way. :) Thanks, Taeung > > 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 >