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
>
