Hi Jiri,
On Fri, Sep 18, 2020 at 10:31 PM Jiri Olsa <jo...@redhat.com> wrote:
>
> 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
ok.
>
> > + 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
Sounds good.
>
> > +
> > + 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
Will do.
Thanks
Namhyung
>
> > +
> > + 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
> >
>
