Messages in this thread | | | Subject | Re: [BUGFIX][PATCH v6 2/9] perf config: If collect_config() is failed, finally free a config set after it is done | From | Taeung Song <> | Date | Tue, 7 Jun 2016 16:44:50 +0900 |
| |
On 06/07/2016 06:37 AM, Taeung Song wrote: > > > On 06/07/2016 05:23 AM, Arnaldo Carvalho de Melo wrote: >> Em Mon, Jun 06, 2016 at 07:52:53PM +0900, Taeung Song escreveu: >>> Because of die() at perf_parse_file() a config set was freed >>> in collect_config(), if failed. >>> But it is natural to free a config set after collect_config() is done >>> when some problems happened. >>> >>> So, in case of failure, lastly free a config set at >>> perf_config_set__new() >>> instead of freeing the config set in collect_config(). >>> >>> Cc: Namhyung Kim <namhyung@kernel.org> >>> Cc: Jiri Olsa <jolsa@redhat.com> >>> Cc: Masami Hiramatsu <mhiramat@kernel.org> >>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com> >>> --- >>> tools/perf/util/config.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >>> index b500737..d013f90 100644 >>> --- a/tools/perf/util/config.c >>> +++ b/tools/perf/util/config.c >>> @@ -639,7 +639,6 @@ static int collect_config(const char *var, const >>> char *value, >>> >>> out_free: >>> free(key); >>> - perf_config_set__delete(set); >>> return -1; >>> } >>> >>> @@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void) >>> >>> if (set) { >>> INIT_LIST_HEAD(&set->sections); >>> - perf_config(collect_config, set); >>> + if (perf_config(collect_config, set) < 0) >>> + perf_config_set__delete(set); >>> } >>> >>> return set; >> >> You can't do that, there is something missing, without looking at the >> code I think you need: >> >> if (set) { >> INIT_LIST_HEAD(&set->sections); >> - perf_config(collect_config, set); >> + if (perf_config(collect_config, set) < 0) { >> + perf_config_set__delete(set); >> + set = NULL; >> + } >> } >> >> return set; >> >> No? >> > > Granted > Sorry for missing above.. > > I modified using 'return NULL;' instead of 'set = NULL;' as below > > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > index c73f1c4..cb749d3 100644 > --- a/tools/perf/util/config.c > +++ b/tools/perf/util/config.c > @@ -643,7 +643,6 @@ static int collect_config(const char *var, const > char *value, > > out_free: > free(key); > - perf_config_set__delete(set); > return -1; > } > > @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) > > if (set) { > INIT_LIST_HEAD(&set->sections); > - perf_config(collect_config, set); > + if (perf_config(collect_config, set) < 0) { > + perf_config_set__delete(set); > + return NULL; > + } > } > > return set; > > Because in near future, perf_config_set__delete() will use zfree(). > > will send changed this patch soon ! > Thank you for your review :) >
Hum.. my answer was stupid. There isn't difference between 'return NULL;' and 'set = NULL;' as a result at perf_config_set__new(). And zfree() at perf_config_set__delete() aren't related to this situation..
Anyway.. I'll send v7 with changed this patch as you said!!
Thanks, Taeung
| |