Re: [PATCH 1/4] perf tools: Introduce hist_entry__init function

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Jul 04, 2016 at 04:01:36PM +0200, Jiri Olsa escreveu:
> Move hist_entry initialization code into separate function.
> It'll be useful and more clear for following patches that
> introduce allocation callbacks.
> 
> Link: http://lkml.kernel.org/n/tip-j8dz6ytir91x8qd1zk4vf7ki@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/hist.c | 142 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 75 insertions(+), 67 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index e1fcc8d7c01a..ed7bea9aa68d 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -352,89 +352,97 @@ void hists__delete_entries(struct hists *hists)
>   * histogram, sorted on item, collects periods
>   */
>  
> -static struct hist_entry *hist_entry__new(struct hist_entry *template,
> -					  bool sample_self)
> +static int hist_entry__init(struct hist_entry *he,
> +			    struct hist_entry *template,
> +			    bool sample_self)
>  {
> -	size_t callchain_size = 0;
> -	struct hist_entry *he;
> +	*he = *template;
>  
> -	if (symbol_conf.use_callchain)
> -		callchain_size = sizeof(struct callchain_root);
> -
> -	he = zalloc(sizeof(*he) + callchain_size);
> +	if (symbol_conf.cumulate_callchain) {
> +		he->stat_acc = malloc(sizeof(he->stat));
> +		if (he->stat_acc == NULL) {
> +			free(he);
> +			return -ENOMEM;
> +		}
> +		memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
> +		if (!sample_self)
> +			memset(&he->stat, 0, sizeof(he->stat));
> +	}
>  
> -	if (he != NULL) {
> -		*he = *template;
> +	map__get(he->ms.map);
>  
> -		if (symbol_conf.cumulate_callchain) {
> -			he->stat_acc = malloc(sizeof(he->stat));
> -			if (he->stat_acc == NULL) {
> -				free(he);
> -				return NULL;
> -			}
> -			memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
> -			if (!sample_self)
> -				memset(&he->stat, 0, sizeof(he->stat));
> +	if (he->branch_info) {
> +		/*
> +		 * This branch info is (a part of) allocated from
> +		 * sample__resolve_bstack() and will be freed after
> +		 * adding new entries.  So we need to save a copy.
> +		 */
> +		he->branch_info = malloc(sizeof(*he->branch_info));
> +		if (he->branch_info == NULL) {
> +			map__zput(he->ms.map);
> +			free(he->stat_acc);
> +			free(he);
> +			return -ENOMEM;
>  		}
>  
> -		map__get(he->ms.map);
> +		memcpy(he->branch_info, template->branch_info,
> +		       sizeof(*he->branch_info));
>  
> -		if (he->branch_info) {
> -			/*
> -			 * This branch info is (a part of) allocated from
> -			 * sample__resolve_bstack() and will be freed after
> -			 * adding new entries.  So we need to save a copy.
> -			 */
> -			he->branch_info = malloc(sizeof(*he->branch_info));
> -			if (he->branch_info == NULL) {
> -				map__zput(he->ms.map);
> -				free(he->stat_acc);
> -				free(he);
> -				return NULL;
> -			}
> +		map__get(he->branch_info->from.map);
> +		map__get(he->branch_info->to.map);
> +	}
>  
> -			memcpy(he->branch_info, template->branch_info,
> -			       sizeof(*he->branch_info));
> +	if (he->mem_info) {
> +		map__get(he->mem_info->iaddr.map);
> +		map__get(he->mem_info->daddr.map);
> +	}
>  
> -			map__get(he->branch_info->from.map);
> -			map__get(he->branch_info->to.map);
> -		}
> +	if (symbol_conf.use_callchain)
> +		callchain_init(he->callchain);
>  
> -		if (he->mem_info) {
> -			map__get(he->mem_info->iaddr.map);
> -			map__get(he->mem_info->daddr.map);
> +	if (he->raw_data) {
> +		he->raw_data = memdup(he->raw_data, he->raw_size);
> +
> +		if (he->raw_data == NULL) {
> +			map__put(he->ms.map);
> +			if (he->branch_info) {
> +				map__put(he->branch_info->from.map);
> +				map__put(he->branch_info->to.map);
> +				free(he->branch_info);
> +			}
> +			if (he->mem_info) {
> +				map__put(he->mem_info->iaddr.map);
> +				map__put(he->mem_info->daddr.map);
> +			}
> +			free(he->stat_acc);
> +			free(he);
> +			return -ENOMEM;
>  		}
> +	}
> +	INIT_LIST_HEAD(&he->pairs.node);
> +	thread__get(he->thread);
>  
> -		if (symbol_conf.use_callchain)
> -			callchain_init(he->callchain);
> +	if (!symbol_conf.report_hierarchy)
> +		he->leaf = true;
>  
> -		if (he->raw_data) {
> -			he->raw_data = memdup(he->raw_data, he->raw_size);
> +	return 0;
> +}
>  
> -			if (he->raw_data == NULL) {
> -				map__put(he->ms.map);
> -				if (he->branch_info) {
> -					map__put(he->branch_info->from.map);
> -					map__put(he->branch_info->to.map);
> -					free(he->branch_info);
> -				}
> -				if (he->mem_info) {
> -					map__put(he->mem_info->iaddr.map);
> -					map__put(he->mem_info->daddr.map);
> -				}
> -				free(he->stat_acc);
> -				free(he);
> -				return NULL;
> -			}
> -		}
> -		INIT_LIST_HEAD(&he->pairs.node);
> -		thread__get(he->thread);
> +static struct hist_entry *hist_entry__new(struct hist_entry *template,
> +					  bool sample_self)
> +{
> +	size_t callchain_size = 0;
> +	struct hist_entry *he;
> +	int err = 0;
>  
> -		if (!symbol_conf.report_hierarchy)
> -			he->leaf = true;
> -	}
> +	if (symbol_conf.use_callchain)
> +		callchain_size = sizeof(struct callchain_root);
>  
> -	return he;
> +	he = zalloc(sizeof(*he) + callchain_size);
> +	if (he)
> +		err = hist_entry__init(he, template, sample_self);

An init function shouldn't free the object being initialized, I haven't
checked if that is the case, the renaming of new->init made the patch
difficult to read, perhaps its just leaking it here in case init()
returns !0, please check.

I.e. if init() somehow fails, then new() should detect that, and free
what new() allocated ('he' in this case).

> +
> +	return err ? NULL : he;
>  }
>  
>  static u8 symbol__parent_filter(const struct symbol *parent)
> -- 
> 2.4.11