Re: [PATCH 1/4] perf evsel: Add evsel__clone() function

[Jiri Olsa]

On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:

SNIP

> +struct evsel *evsel__clone(struct evsel *orig)
> +{
> +	struct evsel *evsel;
> +	struct evsel_config_term *pos, *tmp;
> +
> +	BUG_ON(orig->core.fd);
> +	BUG_ON(orig->counts);
> +	BUG_ON(orig->priv);
> +	BUG_ON(orig->per_pkg_mask);
> +
> +	/* cannot handle BPF objects for now */
> +	if (orig->bpf_obj)
> +		return NULL;
> +
> +	evsel = evsel__new(&orig->core.attr);
> +	if (evsel == NULL)
> +		return NULL;
> +
> +	evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
> +	evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
> +	evsel->core.threads = perf_thread_map__get(orig->core.threads);
> +	evsel->core.nr_members = orig->core.nr_members;
> +	evsel->core.system_wide = orig->core.system_wide;
> +
> +	if (orig->name)
> +		evsel->name = strdup(orig->name);
> +	if (orig->group_name)
> +		evsel->group_name = strdup(orig->group_name);
> +	if (orig->pmu_name)
> +		evsel->pmu_name = strdup(orig->pmu_name);
> +	if (orig->filter)
> +		evsel->filter = strdup(orig->filter);

we should check those strdup results

> +	evsel->cgrp = cgroup__get(orig->cgrp);
> +	evsel->tp_format = orig->tp_format;
> +	evsel->handler = orig->handler;
> +	evsel->leader = orig->leader;
> +
> +	evsel->max_events = orig->max_events;
> +	evsel->tool_event = orig->tool_event;
> +	evsel->unit = orig->unit;
> +	evsel->scale = orig->scale;
> +	evsel->snapshot = orig->snapshot;
> +	evsel->per_pkg = orig->per_pkg;
> +	evsel->percore = orig->percore;
> +	evsel->precise_max = orig->precise_max;
> +	evsel->use_uncore_alias = orig->use_uncore_alias;
> +	evsel->is_libpfm_event = orig->is_libpfm_event;
> +
> +	evsel->exclude_GH = orig->exclude_GH;
> +	evsel->sample_read = orig->sample_read;
> +	evsel->auto_merge_stats = orig->auto_merge_stats;
> +	evsel->collect_stat = orig->collect_stat;
> +	evsel->weak_group = orig->weak_group;

so all those evsel's members are possibly defined in parse time right?
perhaps we should separate them in the struct? and make some note about
evsel__clone function that new members should be considered for copy
in evsel__close.. or something like that

> +
> +	list_for_each_entry(pos, &orig->config_terms, list) {
> +		tmp = malloc(sizeof(*tmp));
> +		if (tmp == NULL) {
> +			evsel__delete(evsel);
> +			evsel = NULL;
> +			break;
> +		}
> +
> +		*tmp = *pos;
> +		if (tmp->free_str) {
> +			tmp->val.str = strdup(pos->val.str);
> +			if (tmp->val.str == NULL) {
> +				evsel__delete(evsel);
> +				evsel = NULL;
> +				free(tmp);
> +				break;
> +			}
> +		}
> +		list_add_tail(&tmp->list, &evsel->config_terms);
> +	}

could this go in separate function? copy_terms

thanks,
jirka

> +
> +	return evsel;
> +}
> +
>  /*
>   * Returns pointer with encoded error via <linux/err.h> interface.
>   */
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 35e3f6d66085..507c31d6a389 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct perf_event_attr *attr)
>  	return evsel__new_idx(attr, 0);
>  }
>  
> +struct evsel *evsel__clone(struct evsel *orig);
>  struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
>  
>  /*
> -- 
> 2.28.0.618.gf4bc123cb7-goog
>