From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751974AbdEHWKE (ORCPT ); Mon, 8 May 2017 18:10:04 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:35837 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751094AbdEHWKC (ORCPT ); Mon, 8 May 2017 18:10:02 -0400 Subject: Re: [PATCH v2 2/4] perf config: Finally write changed configs on config file at a time To: Arnaldo Carvalho de Melo , Taeung Song References: <1494241660-32358-1-git-send-email-treeze.taeung@gmail.com> <20170508163728.GI7891@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim From: Taeung Song Message-ID: <025e2ea8-6b7c-a8e8-2b4f-239879948d38@gmail.com> Date: Tue, 9 May 2017 07:09:58 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170508163728.GI7891@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/09/2017 01:37 AM, Arnaldo Carvalho de Melo wrote: > Em Mon, May 08, 2017 at 08:07:40PM +0900, Taeung Song escreveu: >> Currently set_config() can be repeatedly called for each >> input config on the below case: >> >> $ perf config kmem.default=slab report.children=false ... >> >> But it's a waste, so finally write changed configs at a time. >> >> Cc: Jiri Olsa >> Cc: Namhyung Kim >> Signed-off-by: Taeung Song >> --- >> tools/perf/builtin-config.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >> index 7545966..0d38599 100644 >> --- a/tools/perf/builtin-config.c >> +++ b/tools/perf/builtin-config.c >> @@ -33,8 +33,7 @@ static struct option config_options[] = { >> OPT_END() >> }; >> >> -static int set_config(struct perf_config_set *set, const char *file_name, >> - const char *var, const char *value) >> +static int set_config(struct perf_config_set *set, const char *file_name) >> { >> struct perf_config_section *section = NULL; >> struct perf_config_item *item = NULL; >> @@ -48,7 +47,6 @@ static int set_config(struct perf_config_set *set, const char *file_name, >> if (!fp) >> return -1; >> >> - perf_config_set__collect(set, file_name, var, value); >> fprintf(fp, "%s\n", first_line); >> >> /* overwrite configvariables */ >> @@ -160,6 +158,7 @@ int cmd_config(int argc, const char **argv) >> struct perf_config_set *set; >> char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); >> const char *config_filename; >> + bool changed = false; >> >> argc = parse_options(argc, argv, config_options, config_usage, >> PARSE_OPT_STOP_AT_NON_OPTION); >> @@ -227,10 +226,16 @@ int cmd_config(int argc, const char **argv) >> >> if (value == NULL) >> ret = show_spec_config(set, var); >> - else >> - ret = set_config(set, config_filename, var, value); >> + else { >> + perf_config_set__collect(set, config_filename, >> + var, value); >> + changed = true; >> + } >> free(arg); >> } >> + >> + if (changed) >> + ret = set_config(set, config_filename); > > Ok, this improves the situation, even fixing what seems like a bug, i.e. > that ret variable set in the if (value == NULL) case is not being > checked, the look just goes on to completion and whatever is the last > value left in 'ret' is what will be returned by cmd_config()... > > I think we should fix that bug first, making it exit the loop and > perhaps show some error message, then do this 'call set_config()' only > at the end', no? I understood ! will fix the bug you mentioned first, before this patch. > > I applied the first patch, the one just renesting things. > > - Arnaldo Thank you !, Taeung > >> } >> >> perf_config_set__delete(set); >> -- >> 2.7.4